TST for #1089

[gwarmstrong]

tests for #1089

Summary by CodeRabbit

• Tests
  • Added unit test to strengthen validation of benchmark argument handling. When the executor is configured as "none", the test verifies that input file paths in benchmark arguments correctly resolve to local file system locations instead of container paths.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a new unit test to tests/test_configs.py that validates the behavior of get_benchmark_args_from_module when the executor is set to "none". The test ensures that the input_file resolves to a local file path rather than a container path.

Changes

Cohort / File(s)	Change Summary
Test Module Updates tests/test_configs.py	Added two imports (SimpleNamespace from types and get_benchmark_args_from_module from nemo_skills.pipeline.utils.eval) and new test function test_get_benchmark_args_input_file_should_be_local_path_for_executor_none to validate local path resolution when executor is "none"

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

• Single test addition with straightforward assertions
• Minimal new code with no complex logic
• Review focus: Verify test assertion logic and that mock setup correctly mimics the intended scenario

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title "TST for #1089" is vague and does not clearly convey the specific change being made. While it references an issue number, it uses the non-descriptive abbreviation "TST" and provides insufficient context about what is being tested.	Replace the generic abbreviation with a descriptive title like "Add unit test validating local path resolution for executor none" or "Add test for benchmark args input_file with executor set to none".

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch georgea/add-none-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]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/test_configs.py (1)

85-121: Test correctly asserts local input_file behavior; consider slightly richer cluster_config

The test setup and assertion look solid: creating tmp_path / "gsm8k" / "test.jsonl" and checking result.input_file against that exact path is a good regression check for the executor='none' behavior of get_benchmark_args_from_module.

To make the test a bit more future‑proof against internal changes in path/mount handling, you might consider mirroring the typical cluster_config shape by including an empty mounts list:

-    cluster_config = {"executor": "none", "containers": {}}
+    cluster_config = {"executor": "none", "containers": {}, "mounts": []}

This keeps the intent the same while reducing the chance of KeyErrors if helper functions start assuming mounts is present.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 42eded0 and b2999cd.

📒 Files selected for processing (1)

• tests/test_configs.py (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tests/test_configs.py (2)

nemo_skills/pipeline/utils/mounts.py (1)

• get_mounted_path (165-210)

nemo_skills/pipeline/utils/eval.py (1)

• get_benchmark_args_from_module (79-187)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: unit-tests
• GitHub Check: pre-commit

🔇 Additional comments (1)

tests/test_configs.py (1)

16-22: New imports are appropriate for the added test

Using SimpleNamespace to mock the benchmark module and importing get_benchmark_args_from_module directly keeps the test focused and aligned with the real API. No issues here.