Support path-based test references in scenario files - #994
Open
shreyaskommuri wants to merge 3 commits into
Open
Support path-based test references in scenario files#994shreyaskommuri wants to merge 3 commits into
shreyaskommuri wants to merge 3 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>
shreyaskommuri
requested review from
jj10306,
podkidyshev,
rutayan-nv and
srivatsankrishnan
as code owners
August 10, 2026 21:06
Contributor
|
Caution Review failedFailed to post review comments. GitHub was unavailable or timed out while CodeRabbit was posting the review. Please request a new review later if the pull request still needs one. This happened while posting 1 inline comment. Use ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 11 remain after this review. 🧰 Additional context used🧠 Learnings (11)📚 Learning: 2025-12-16T19:47:41.994ZApplied to files:
📚 Learning: 2026-03-16T14:10:11.280ZApplied to files:
📚 Learning: 2026-03-24T23:24:17.668ZApplied to files:
📚 Learning: 2026-04-14T11:05:59.444ZApplied to files:
📚 Learning: 2026-06-07T12:01:43.285ZApplied to files:
📚 Learning: 2026-06-24T01:05:39.500ZApplied to files:
📚 Learning: 2026-02-10T13:29:25.671ZApplied to files:
📚 Learning: 2026-03-10T11:01:25.158ZApplied to files:
📚 Learning: 2026-03-17T20:01:53.137ZApplied to files:
📚 Learning: 2026-02-12T13:47:34.298ZApplied to files:
📚 Learning: 2026-03-24T14:24:38.412ZApplied to files:
🪛 Ruff (0.16.1)src/cloudai/models/scenario.py[warning] 157-158: Use Convert to (PLR5501) 📝 WalkthroughWalkthrough
ChangesScenario test path references
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The change is localized and backward-compatible, but an absolute path may bypass the documented scenario-file-relative resolution and load a file outside that directory; confirm or restrict this behavior before merging. 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}
Contributor
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>
Contributor
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
Contributor
Author
|
@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. |
- 'test_template_name' must not be set if 'test_name' or 'path' is set now checks `is not None` instead of truthiness, so an explicit empty string no longer slips past the mutual-exclusion check. - test_empty_path_is_rejected and the new empty-test_template_name test pass their match pattern directly to pytest.raises instead of a separate exc_info.match() call. - test_full_scenario_toml_with_path_reference now calls the public TestScenarioParser.parse() instead of the private _prepare_tdef(), exercising the actual end-to-end scenario-parsing path. Signed-off-by: shreyaskommuri <shreyaskommuri@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.