test(rules): align generate_output_location tests with current args.* contract#557
Conversation
… contract
The test file (test_generate_output_location.py) was written when
generate_output_location's design accepted orgname/systemname as
keyword-only kwargs threaded by the CLI dispatch layer. The function
has since been rewritten (LAY-05) to read both from benchmark.args,
which is populated upstream by main._main_impl()'s orgname-resolution
gate (reads orgname.yaml written by `mlpstorage init`) and argparse
from --systemname.
The 25 tests in the file kept passing values as Python kwargs that the
function silently ignored, so every test hit the "orgname is empty"
ConfigurationError before reaching the behavior under test.
Changes:
mlpstorage_py/rules/utils.py
- Remove unused orgname=/systemname= keyword-only parameters from the
function signature. The body already reads args.orgname/args.systemname
exclusively; keeping the kwargs in the signature misled the test
author (the failure cascade for this whole file).
- Add parameter="orgname" / parameter="systemname" to the two existing
ConfigurationError raises so the dispatch layer can surface a typed
user-facing error (the ConfigurationError class already exposes the
attribute; the raise sites just weren't filling it in).
- Fix the systemname suggestion env-var name from MLPERF_SYSTEMNAME to
MLPSTORAGE_SYSTEMNAME to match the canonical
MLPSTORAGE_SYSTEMNAME_ENVVAR constant exported by the same module.
mlpstorage_py/benchmarks/base.py
- Simplify the one production caller that was threading
args._validated_orgname / args._validated_systemname through as kwargs.
Those kwargs are gone now and the comment about "legacy / whatif modes
skip the orgname/systemname requirement" no longer matches production
behavior (whatif uses the same canonical shape as closed/open).
The validation helper still stashes _validated_* on args; only the
thread-through-as-kwargs step is removed.
mlpstorage_py/tests/test_generate_output_location.py
- Rewrite _benchmark() helper to put orgname/systemname on args by
default (matching the production contract), with overrides for omit
(None) and empty-string ("") cases.
- All 19 happy-path / safety-guard tests drop the inline `orgname=`/
`systemname=` kwargs.
- Drop test_missing_mode_attribute_keeps_legacy_shape and rename
test_whatif_has_no_closed_open_prefix to
test_whatif_uses_canonical_shape — production no longer special-cases
either mode-missing or whatif.
- The missing-orgname/empty-orgname/missing-systemname tests now omit
the attribute from args (the realistic failure mode at runtime) and
assert the typed `parameter` field that the production code now sets.
Result: 25 previously-failing tests in this file → 32 passing (file
gained one test from a parametrize-out collapse).
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
…location call assertion PR #557 (this branch) removed the unused `orgname=`/`systemname=` keyword-only params from `generate_output_location`'s signature and updated the production call site in `mlpstorage_py/benchmarks/base.py` to stop threading them through. The primary tests in `mlpstorage_py/tests/test_generate_output_location.py` were updated to match, but a sibling test in `tests/unit/test_benchmarks_base.py` (a separate test tree) still asserted the old kwarg-bearing call shape via `assert_called_once_with(benchmark, datetime_str, orgname=None, systemname=None)`, which `mock.assert_called_once_with` treats as a literal equality check on the call signature. After the production code dropped the kwargs the mock saw `generate_output_location(benchmark, datetime_str)` and the assertion failed with "expected call not found". The fix is to drop the dead kwargs from the assertion; the behavior under test is just "the production wrapper calls the helper with the benchmark and a datetime string."
|
Heads-up from a parallel session: pushed a one-line fix to this branch (commit `35954a0`) that unsticks CI. What was broken: The fix: drop the dead kwargs from the assertion at line 580 — the behavior under test is just "wrapper calls helper with the benchmark and a datetime string." - mock_gen.assert_called_once_with(
- benchmark, "20250115_120000", orgname=None, systemname=None
- )
+ mock_gen.assert_called_once_with(benchmark, "20250115_120000")Why I touched this branch from a sibling session: the user was tracking three concurrent PRs (#553, #555, #557). #553 and #555 went green; #557 was the only one with a real test failure (vs runner flakes) and I had context loaded, so unsticking it inline was faster than a hand-off. CI is green now ( If you'd rather restructure the fix (e.g. fold it into the existing |
2 participants
Summary
`generate_output_location` was rewritten (LAY-05) to read `args.orgname` and `args.systemname` directly, populated upstream by:
The test file in `mlpstorage_py/tests/test_generate_output_location.py` predated that rewrite and still passed `orgname=`/`systemname=` as Python kwargs that the function silently ignored, so every one of 25 tests hit the "orgname is empty" `ConfigurationError` before reaching the behavior under test. The unused kwargs in the function signature were the trap that misled the test author.
Changes
Test plan