Support path-based test references in scenario files - #994
Support path-based test references in scenario files#994shreyaskommuri wants to merge 2 commits into
Conversation
Add an optional 'path' field on TestRunModel, mutually exclusive with 'test_name', resolved relative to the scenario file's own directory. When set, _prepare_tdef loads that file directly instead of doing a name lookup against test_mapping, with the same scenario-level override merging test_name already supports. Ref: NVIDIA#985 Signed-off-by: shreyaskommuri <shreyaskommuri@gmail.com>
📝 WalkthroughWalkthrough
ChangesScenario test path references
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cloudai/models/scenario.py`:
- Around line 74-80: Update the Optional path field in TestRunModel to enforce
min_length=1, rejecting empty strings during validation while preserving None as
valid. Add a regression test covering path="" and verify validation fails before
_prepare_tdef is reached.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5c512b3e-96c4-4630-84b6-898ffd08b847
📒 Files selected for processing (3)
src/cloudai/models/scenario.pysrc/cloudai/test_scenario_parser.pytests/test_test_scenario.py
Field(min_length=1) rejects path="" at validation time instead of letting it through as a truthy-looking value that would otherwise resolve to the scenario file's own directory. Signed-off-by: shreyaskommuri <shreyaskommuri@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_test_scenario.py (1)
1006-1032: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the public scenario parser in the end-to-end test.
At Lines 1027-1030, the test manually validates the TOML and calls private
_prepare_tdef(). This verifies the helper and the model, but not the parser's public scenario-loading flow. If this test must provide end-to-end coverage, use the public parser entry point and assert the resulting scenario test. Otherwise, rename the test to indicate helper-level coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_test_scenario.py` around lines 1006 - 1032, Update test_full_scenario_toml_with_path_reference to exercise TestScenarioParser’s public scenario-loading entry point instead of manually calling TestScenarioModel.model_validate and private _prepare_tdef. Assert the resulting parsed scenario test still has name "nccl", preserving end-to-end coverage of the path reference flow.src/cloudai/models/scenario.py (1)
159-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject an empty
test_template_namewith a path.
if self.test_template_nametreats""as unset. Therefore,TestRunModel(id="1", path="nccl.toml", test_template_name="")passes validation, althoughpathandtest_template_nameare mutually exclusive.tdef_model_dump()retains the empty string, so_prepare_tdef()can merge it over the referenced definition. Use anis not Nonecheck and add a regression test.Proposed validation fix
else: - if self.test_template_name: + if self.test_template_name is not None: raise ValueError("'test_template_name' must not be set if 'test_name' or 'path' is set.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cloudai/models/scenario.py` at line 159, Update the validation involving test_template_name in TestRunModel so an empty string is treated as explicitly set when path or test_name is provided, using an is not None check rather than truthiness; add a regression test covering path with test_template_name="".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_test_scenario.py`:
- Around line 944-947: Update test_empty_path_is_rejected to pass the expected
error-message pattern directly via pytest.raises, and remove the separate
exc_info.match assertion.
---
Outside diff comments:
In `@src/cloudai/models/scenario.py`:
- Line 159: Update the validation involving test_template_name in TestRunModel
so an empty string is treated as explicitly set when path or test_name is
provided, using an is not None check rather than truthiness; add a regression
test covering path with test_template_name="".
In `@tests/test_test_scenario.py`:
- Around line 1006-1032: Update test_full_scenario_toml_with_path_reference to
exercise TestScenarioParser’s public scenario-loading entry point instead of
manually calling TestScenarioModel.model_validate and private _prepare_tdef.
Assert the resulting parsed scenario test still has name "nccl", preserving
end-to-end coverage of the path reference flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 171803a1-2257-46ce-b0ea-5491d6e62d78
📒 Files selected for processing (2)
src/cloudai/models/scenario.pytests/test_test_scenario.py
| def test_empty_path_is_rejected(self): | ||
| with pytest.raises(ValueError) as exc_info: | ||
| TestRunModel(id="1", path="") | ||
| assert exc_info.match("String should have at least 1 character") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target test ---'
sed -n '930,955p' tests/test_test_scenario.py
printf '%s\n' '--- Ruff configuration and PT011 references ---'
rg -n --hidden -g '!*.lock' -g '!node_modules' -g '!dist' \
'PT011|flake8-pytest|pytest.raises|ruff' pyproject.toml setup.cfg tox.ini .ruff.toml ruff.toml tests 2>/dev/null || true
printf '%s\n' '--- Ruff availability ---'
if command -v ruff >/dev/null 2>&1; then
ruff --version
ruff check --select PT011 tests/test_test_scenario.py
else
echo 'ruff is not available'
fiRepository: NVIDIA/cloudai
Length of output: 24029
Pass the message to pytest.raises.
Ruff PT011 flags this test. Move the message match into pytest.raises and remove the separate assertion.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 945-945: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_test_scenario.py` around lines 944 - 947, Update
test_empty_path_is_rejected to pass the expected error-message pattern directly
via pytest.raises, and remove the separate exc_info.match assertion.
Source: Linters/SAST tools
|
@podkidyshev the empty-path issue from CodeRabbit is resolved, min_length=1 pushed and confirmed rejecting "" while still allowing None/real paths (see reply on the review thread above), plus a regression test. All CI green on the latest commit. Let me know if there's anything else you'd like changed. |
2 participants
Summary
pathfield onTestRunModel, an alternative totest_namefor referencing a test in a scenario file, resolved relative to the scenario file's own directory (confirmed shape in Support relative-path test references and lazy-load only referenced test/hook tomls #985).test_nameandpathare mutually exclusive, same rulestest_namealready has withtest_template_name.TestScenarioParser._prepare_tdefgets a new branch: whenpathis set, it loads that toml file directly instead of doing a name lookup againsttest_mapping, then merges scenario-level overrides the same way thetest_namebranch already does.TestScenarioParsingErrorif the resolved path does not exist.--tests-dir/hooks when a scenario's tests are all path-referenced) depends on this one and is not included here, keeping this PR small and reviewable on its own.test_nameand fully-inline (test_template_name+name+description) scenarios are unaffected.Test Plan
7 new tests in
tests/test_test_scenario.py(TestPathReference), covering:path/test_namemutual exclusion,path/test_template_namemutual exclusion, relative-path resolution against the scenario file's directory, scenario-level override merging over the referenced file, a missing-file error, and a full scenario TOML parsed end to end with apathreference.Also updated the wording of two pre-existing validation error messages (in
models/scenario.py) that referenced onlytest_name, so they stay accurate now thatpathis a second way to satisfy the same requirement. Updated the two existing tests intests/test_test_scenario.pythat asserted on the old wording.Additional Notes
Design confirmed in #985 before writing any code, per
CONTRIBUTING.md's "communicate with the main developers before starting work." Not touching the eager-loading behavior itself, sweeping, or anything unrelated, this PR is purely additive.