Support path-based test references in scenario files - #994
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 Possibly related issues
Possibly related PRs
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
| path: Optional[str] = Field( | ||
| default=None, | ||
| description=( | ||
| "Path to a test TOML file, resolved relative to this scenario file's own directory. " | ||
| "Alternative to 'test_name': references a test by file location instead of by name." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For Pydantic 2.12, does Field(min_length=1) reject an empty string while allowing None for a str | None field?
💡 Result:
Yes, in Pydantic 2.12, Field(min_length=1) will reject an empty string while allowing None for a str | None field [1][2][3]. When you define a field as str | None (or Optional[str]), Pydantic understands that the field accepts either a string or a None value [2]. Constraints applied via Field—such as min_length=1—are automatically applied specifically to the string member of the union [1][2]. Consequently: - An input of None will satisfy the validation (assuming the field is nullable) [1][3]. - An input of "" (empty string) will be validated against the min_length=1 constraint, which it will fail, triggering a validation error [4][3]. This behavior is standard for Pydantic V2 when working with Union types containing None [2]. If you ever need an empty string to be treated as None, you must handle that conversion explicitly using a validator, as Pydantic treats an empty string as a valid string value, not a missing value [5][6].
Citations:
- 1: Field constraints fail when using None as generic argument for a Union candidate pydantic/pydantic#7291
- 2: https://pydantic.dev/docs/validation/latest/concepts/fields/
- 3: https://stackoverflow.com/questions/77348076/optional-str-in-pydantic-2-0-with-field-constraints
- 4: Field add not_allow_empty option pydantic/pydantic#4598
- 5: Pydantic model field: convert empty string to None pydantic/pydantic#2687
- 6: https://stackoverflow.com/questions/59584142/pydantic-validator-to-remove-blank-strings
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(scenario\.py|pyproject\.toml|.*test.*scenario.*|.*parser.*\.py)$' | head -100
printf '%s\n' '--- scenario outline ---'
ast-grep outline src/cloudai/models/scenario.py 2>/dev/null || true
printf '%s\n' '--- relevant symbols ---'
rg -n -C 5 'class TestScenarioParser|def _prepare_tdef|test_info\.path|path: Optional\[str\]|min_length|pydantic' src pyproject.toml 2>/dev/null | head -240Repository: NVIDIA/cloudai
Length of output: 19582
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- scenario model ---'
sed -n '54,175p' src/cloudai/models/scenario.py
printf '%s\n' '--- parser method ---'
sed -n '205,270p' src/cloudai/test_scenario_parser.py
printf '%s\n' '--- scenario tests ---'
rg -n -C 6 'path|test_name|TestRunModel|TestScenarioParsingError|ValueError' tests/test_test_scenario.py tests/test_test_scenario_parser.py
printf '%s\n' '--- isolated Pydantic behavior ---'
python3 - <<'PY'
try:
import pydantic
from typing import Optional
from pydantic import BaseModel, Field, ValidationError
print("pydantic_version=", pydantic.__version__)
class Probe(BaseModel):
path: Optional[str] = Field(default=None, min_length=1)
for value in (None, "", "x"):
try:
result = Probe(path=value)
print(repr(value), "accepted", repr(result.path))
except ValidationError as exc:
print(repr(value), "rejected", exc.errors()[0]["type"])
except Exception as exc:
print(type(exc).__name__ + ":", exc)
PYRepository: NVIDIA/cloudai
Length of output: 45874
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- parser callers and error handling ---'
rg -n -C 8 '_prepare_tdef|TestScenarioParsingError|ValidationError|parse_data|_create_test_run' src/cloudai/test_scenario_parser.py src/cloudai/parser.py src/cloudai/_core tests/test_test_scenario.py | head -320
printf '%s\n' '--- exception definitions ---'
rg -n -C 8 'class TestScenarioParsingError|class .*ParsingError' src/cloudaiRepository: NVIDIA/cloudai
Length of output: 25181
Reject empty path values.
When path="", TestRunModel accepts the value, but _prepare_tdef selects no branch and raises an unwrapped ValueError. Set min_length=1 and add a regression test.
🤖 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` around lines 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.
1 participant
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.