Fix: Parametrize transformation tests and anchor pytest environment#3391
Merged
Conversation
The transformation suite had three usability defects that all surface as
"the tests silently lie about what they're testing":
1. From any cwd other than tests/, glob.glob('topology/input/*yml')
returned [], the for-loop ran zero iterations, and pytest reported
PASSED. A test file globbing relative paths cannot be safely invoked
from anywhere except tests/, but AGENTS.md showed exactly that.
2. From a git worktree, 'cd tests && pytest' silently imported the
netsim package from the venv egg-link (the main checkout) instead of
the worktree's own code, because sys.path[0]='' resolves to cwd at
interpreter startup and tests/ has no netsim/. The wrapper scripts
papered over this with PYTHONPATH=../ but bare pytest did not.
3. All 109 transform cases (and 110 error cases, and the two coverage
suites) ran inside a single for-loop with one assert. The first
failing case aborted the rest -- CI and developers saw "1 failed"
without knowing whether the regression touched 1 case or 50, and -k
couldn't target an individual case because they weren't pytest nodes.
Changes:
- New tests/conftest.py: prepends the worktree root to sys.path so
imports always resolve against this checkout's netsim/, and chdir's
to tests/ in pytest_configure so the existing relative globs work
from any invocation cwd.
- tests/test_transformation.py: @pytest.mark.parametrize over the four
glob.glob() lists so each YAML case becomes its own pytest node
(test_xform_cases[topology/input/foo.yml], etc.). -k can now target
one case; one failure no longer hides others. test_coverage_verbose_cases
iterates the glob directly since it's no longer callable as a function
after parametrize. The unreachable __main__ block (broken signature,
no callers) was removed.
The run-tests.sh / run-xform.sh / run-xerr.sh / run-coverage-tests.sh
wrappers continue to work unchanged; their -k selectors keep matching
because the function names are preserved.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
8 tasks
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
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.
This is the first out of two PRs to make tests behavior more predictable and robust.
Summary
tests/test_transformation.pynow uses@pytest.mark.parametrizeso each YAML case is its own pytest node — one failing case no longer hides the rest, and-kcan target an individual case.tests/conftest.pyanchorssys.pathand the working directory so pytest produces correct results regardless of where it is invoked from, including from inside a git worktree.Why
Three usability defects in the existing harness all amount to "the tests silently lie about what they are testing." This PR fixes the structural half of them; an output-format follow-up is described below.
1. Silent PASS when invoked from the wrong cwd
glob.glob('topology/input/*yml')returns[]when pytest is run from anywhere other thantests/— theforloop runs zero iterations, theassertnever executes, pytest reports PASSED. AGENTS.md showedpython3 -m pytest -vvv -k 'xform_ or error_cases'from the repo root, which means following the documentation produced a silent no-op.2. Silent execution of the wrong netsim from worktrees
A subtler variant of the same theme: from a git worktree,
cd tests && python3 -m pytest …silently imports thenetsimpackage from the venv'spip install -e .egg-link — which points at the main checkout, not the worktree. Tests then run main-checkout code against the worktree's fixtures. The output looked plausible (sometimes 1 failure, sometimes 29, depending on how far the two checkouts had drifted apart).run-tests.shpapered over this withPYTHONPATH=../but bare pytest invocations did not.This is not the same problem as item 1 — it specifically bites people who use worktrees, and it bites them precisely when they follow the "run from
tests/" advice that fixes item 1.3. First failing case aborts the rest
All 109 transform cases (and 110 error cases, 22+30 coverage cases) run inside a single
forloop with oneassert. The first mismatch raises, the loop exits, the remaining ~108 cases never execute. CI sees "1 failed" without knowing whether a regression touched 1 case or 50.-kcannot target individual cases because they are not pytest nodes.What changes
tests/conftest.pysys.pathat conftest load, so the worktree's ownnetsim/always wins over the egg-link.os.chdirs totests/inpytest_configure, so the existing relative globs resolve from any invocation cwd.glob.glob(...)and thefrom netsim import augmentline both see the right environment.tests/test_transformation.py.@pytest.mark.parametrizeover each of the fourglob.glob()lists. Every YAML case becomes its own pytest node —test_xform_cases[topology/input/foo.yml], etc.-ktargets a single case; one failure no longer hides others.test_coverage_verbose_casesiterates the glob directly inside its body, since it can no longer calltest_xform_casesas a plain function after parametrize.__main__block at the bottom (broken signature, no callers) is removed.The
run-tests.sh/run-xform.sh/run-xerr.sh/run-coverage-tests.shwrappers continue to work unchanged. Their-kselectors keep matching because the test function names are preserved.What is NOT in this PR (planned follow-up)
A follow-up branch (
test-harness-aggressive) addresses the related concern that the test bodies bypass pytest's own output handling — theyprint("Test case: …")per case and manuallydifflib.unified_diffon failure, both of which duplicate what pytest already does via-vand assertion rewriting. The follow-up also adoptstmp_pathfor per-case file isolation (soansible_inventory's CWD-relativegroup_vars//host_vars/writes land in a per-test temp dir) and retires therm -fr *filescleanup hack in the wrappers.That work is split out deliberately. This PR makes only structural changes to the harness; output format is a separate conversation.
Anticipated questions
sys.path.insert? Shouldn't the wrappers handle that?" They already do, viaPYTHONPATH=../. But direct pytest invocations (which AGENTS.md previously suggested and which IDE runners typically use) bypassed that and silently ran the wrong code in worktrees. The conftest line is two lines that close that hole regardless of how pytest is launched.glob.glob(...)returned filesystem-insertion order, which varies across machines and clones; the new code wraps it insorted(...), so ordering is now alphabetical and stable. The change reduces, not increases, non-determinism.Test plan
python3 -m pytest -vvv -k 'xform_ or error_cases'— expect 219 passed, 55 deselected. This is the case that previously produced a silent PASS with zero cases collected; it now works because of the conftest cwd anchor.cd tests && python3 -m pytest -vvv -k 'xform_ or error_cases'— expect 219 passed, 55 deselected. The historical contract still works.python3 -m pytest --collect-only -q -k xform_ <repo>/tests/test_transformation.py— expect 109 cases collected, not 0 (validates the silent-PASS-from-wrong-cwd fix).cd tests && ./run-tests.sh— expect 219 passed (validates wrappers still work).python3 -m pytest -vvv 'tests/test_transformation.py::test_xform_cases[topology/input/6pe.yml]'from repo root — expect 1 passed (validates per-case targetability).tests/topology/expected/6pe.yml), re-run the suite, confirm pytest reports "1 failed, 108 passed" instead of aborting at the first mismatch. Revert.cd tests && python3 -m pytest -k xform_and confirm it does not silently use the main checkout's netsim (smoke-test by introducing a deliberate divergence between the two checkouts).