Add unit tests for utility functions

[csahithi]

Summary

• Adds tests/test_utils.py covering 10 previously untested functions across four modules: submit.py, config.py, slurm.py,
  and sglang.py
• 48 tests total, all passing

Coverage added

Module	Functions tested
cli/submit.py	get_job_name, is_sweep_config, find_yaml_files
core/config.py	load_cluster_config, resolve_config_with_defaults, validate_config_file
core/slurm.py	get_slurm_job_id, get_slurm_nodelist, get_container_mounts_str
backends/sglang.py	_config_to_cli_args

Test plan

• make check passes (lint + all tests)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Enhanced test coverage for core utility functions including configuration loading, YAML file discovery, SLURM environment handling, container mounting, and CLI argument generation.

Note: This release contains no user-visible changes—all updates are internal test enhancements.

[coderabbitai]

📝 Walkthrough

Walkthrough

A new test module is added containing unit tests for core utility functions spanning submission configuration, SLURM environment handling, container mount formatting, and CLI argument construction. Tests validate environment variable precedence, YAML file discovery, node expansion, and argument transformation logic.

Changes

Cohort / File(s)

Summary

Test Utilities Coverage tests/test_utils.py

New test module with 381 lines covering utility helpers: get_job_name, is_sweep_config, find_yaml_files, load_cluster_config, resolve_config_with_defaults, validate_config_file, get_slurm_job_id, get_slurm_nodelist, get_container_mounts_str, and _config_to_cli_args. Tests verify environment variable behavior, YAML traversal, SLURM node expansion with mocked subprocess, CLI argument mapping, and error handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hoppy tests now guard the code so true,
With every function covered, shiny new!
SLURM and YAML dance in harmony,
CLI args transform with perfect symmetry,
Quality takes a leap—three hundred eighty-one!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Add unit tests for utility functions' directly and clearly describes the main change—adding a new test file (tests/test_utils.py) with 48 tests for utility functions across multiple modules.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_utils.py (1)

294-319: Consider patching at the import site for more targeted mocking.

The current approach patches subprocess.run globally. While this works correctly, patching at the import site (srtctl.core.slurm.subprocess.run) is a best practice that ensures the mock only affects the module under test and doesn't inadvertently impact other code that might run during the test.

♻️ Suggested improvement

     def test_expands_nodelist_via_scontrol(self, monkeypatch):
         monkeypatch.setenv("SLURM_NODELIST", "h100-[01-03]")
         mock_result = MagicMock()
         mock_result.stdout = "h100-01\nh100-02\nh100-03"
         mock_result.returncode = 0
-        with patch("subprocess.run", return_value=mock_result) as mock_run:
+        with patch("srtctl.core.slurm.subprocess.run", return_value=mock_result) as mock_run:
             nodes = get_slurm_nodelist()
         assert nodes == ["h100-01", "h100-02", "h100-03"]
         mock_run.assert_called_once_with(
             ["scontrol", "show", "hostnames", "h100-[01-03]"],
             capture_output=True,
             text=True,
             check=True,
         )

     def test_falls_back_to_raw_nodelist_on_scontrol_failure(self, monkeypatch):
         monkeypatch.setenv("SLURM_NODELIST", "single-node")
-        with patch("subprocess.run", side_effect=FileNotFoundError):
+        with patch("srtctl.core.slurm.subprocess.run", side_effect=FileNotFoundError):
             nodes = get_slurm_nodelist()
         assert nodes == ["single-node"]

     def test_falls_back_on_subprocess_error(self, monkeypatch):
         monkeypatch.setenv("SLURM_NODELIST", "node01")
-        with patch("subprocess.run", side_effect=subprocess.CalledProcessError(1, "scontrol")):
+        with patch("srtctl.core.slurm.subprocess.run", side_effect=subprocess.CalledProcessError(1, "scontrol")):
             nodes = get_slurm_nodelist()
         assert nodes == ["node01"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_utils.py` around lines 294 - 319, Tests patch subprocess.run
globally which can affect other modules; change the patch target to the import
site used by the code under test. Update the three tests (those calling
get_slurm_nodelist) to patch "srtctl.core.slurm.subprocess.run" instead of
"subprocess.run" so the mock only replaces the subprocess.run used inside
get_slurm_nodelist; keep the same return_value/side_effect behavior and
assertions (including mock_run.assert_called_once_with and the expected node
lists).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/test_utils.py`:
- Around line 294-319: Tests patch subprocess.run globally which can affect
other modules; change the patch target to the import site used by the code under
test. Update the three tests (those calling get_slurm_nodelist) to patch
"srtctl.core.slurm.subprocess.run" instead of "subprocess.run" so the mock only
replaces the subprocess.run used inside get_slurm_nodelist; keep the same
return_value/side_effect behavior and assertions (including
mock_run.assert_called_once_with and the expected node lists).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fcba30f0-3876-4d3b-ba14-2447dc98652d

📥 Commits

Reviewing files that changed from the base of the PR and between 4a22a18 and 84c3946.

📒 Files selected for processing (1)

• tests/test_utils.py