From d20440886ccdbc52cd36631e61bb7435798d81d2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 14:46:58 +0000 Subject: [PATCH 1/2] symmetry: a comparison that could not run fails CI instead of passing (#34) test-symmetry.yml reported green on #33 with every video comparison unrun: its NDR-matlab branch was pushed after the run had fallen back to main, Python's read tests skipped (hidden among the 16 skips every run carried by design), and MATLAB's read tests disp+returned, which counts as passed. Fix all three paths, following the bridge tests' rule: - tests/symmetry/conftest.py: under NDR_SYMMETRY_CHECK_STRICT, any skip in a symmetry test fails, with the reason and what to check. A backstop, not a list, like tests/conftest.py's for the bridge. - tests/test_symmetry_strict.py: positive controls in the normal matrix. A real read test against an empty artifact base skips without the flag and fails with it (and fails the control when the backstop is removed). - test-symmetry.yml: - sets NDR_SYMMETRY_CHECK_STRICT for the job; - writes the resolved NDR-matlab ref and why to the job summary; - Stage 2 deselects the pythonArtifacts cases (made in Stage 3) rather than skipping them, so it now reports 0 skipped; - both MATLAB stages fail on any Incomplete result; - Stage 4 deselects by name the one known gap, readOmeZarr's pythonArtifacts case (NDR-python has no OME-Zarr symmetry test yet), and logs it. - AGENTS.md: the fourth entry in "A skip is a silent pass". - read_artifacts/INSTRUCTIONS.md: skips fail in CI; how to run as CI does. Paired with NDR-matlab on the same branch name, where the read tests now end in assumptions (Incomplete) instead of disp+return (Passed). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01QjkvVXsdCVw6NNe5SnGNyX --- .github/workflows/test-symmetry.yml | 73 +++++++++++++++++-- AGENTS.md | 18 ++++- tests/symmetry/conftest.py | 51 +++++++++++++ tests/symmetry/read_artifacts/INSTRUCTIONS.md | 12 ++- tests/test_symmetry_strict.py | 55 ++++++++++++++ 5 files changed, 199 insertions(+), 10 deletions(-) create mode 100644 tests/test_symmetry_strict.py diff --git a/.github/workflows/test-symmetry.yml b/.github/workflows/test-symmetry.yml index cdc1c79..27e496f 100644 --- a/.github/workflows/test-symmetry.yml +++ b/.github/workflows/test-symmetry.yml @@ -15,6 +15,13 @@ jobs: symmetry: name: MATLAB <-> Python symmetry tests runs-on: ubuntu-latest + env: + # Every comparison is expected to run here, so a comparison that could + # not run FAILS instead of reporting green: a skipped Python symmetry + # test fails (tests/symmetry/conftest.py), and the MATLAB stages fail + # on any Incomplete result. Cases that cannot run in a given stage by + # design are deselected by name in that stage. VH-Lab/NDR-python#34. + NDR_SYMMETRY_CHECK_STRICT: "1" steps: # -- Checkout both repos ---------------------------------------------- @@ -28,15 +35,29 @@ jobs: # Developers are expected to keep matching branch names across the # two repos when making cross-language changes. Falls back to main # when the matching branch does not exist in NDR-matlab. + # + # The choice is written to the job summary with its reason: a paired + # change whose NDR-matlab branch was pushed AFTER this run started + # resolves to main, and then fails in strict mode for want of the + # MATLAB half -- push NDR-matlab first, or re-run once it exists. - name: Resolve NDR-matlab branch id: matlab-ref run: | BRANCH="${{ github.head_ref || github.ref_name }}" if git ls-remote --exit-code --heads https://github.com/VH-Lab/NDR-matlab.git "$BRANCH" >/dev/null 2>&1; then - echo "ref=$BRANCH" >> "$GITHUB_OUTPUT" + REF="$BRANCH" + WHY="NDR-matlab has a branch named \`$BRANCH\`, matching this run." else - echo "ref=main" >> "$GITHUB_OUTPUT" + REF="main" + WHY="NDR-matlab has no branch named \`$BRANCH\`, so this run falls back to \`main\`. If this is a paired change, push the NDR-matlab branch and re-run." fi + echo "ref=$REF" >> "$GITHUB_OUTPUT" + echo "NDR-matlab ref: $REF ($WHY)" + { + echo "### NDR-matlab checkout: \`$REF\`" + echo + echo "$WHY" + } >> "$GITHUB_STEP_SUMMARY" - name: Check out NDR-matlab (matching branch) uses: actions/checkout@v4 @@ -107,15 +128,29 @@ jobs: runner = TestRunner.withTextOutput("Verbosity", "Detailed"); results = runner.run(suite); - fprintf("\n=== MATLAB makeArtifacts: %d passed, %d failed ===\n", ... - nnz([results.Passed]), nnz([results.Failed])); + fprintf("\n=== MATLAB makeArtifacts: %d passed, %d failed, %d incomplete ===\n", ... + nnz([results.Passed]), nnz([results.Failed]), nnz([results.Incomplete])); assert(all(~[results.Failed]), "MATLAB makeArtifacts failed") + % Incomplete = an assumption failed (a missing example file or + % tool), so these artifacts were never made. Strict mode: fail. + incomplete = results([results.Incomplete]); + for k = 1:numel(incomplete) + fprintf(2, "INCOMPLETE: %s\n", incomplete(k).Name); + end + assert(isempty(incomplete), ... + "%d MATLAB makeArtifacts test(s) were Incomplete: their artifacts were never made.", ... + numel(incomplete)) + # -- Stage 2: Python readArtifacts (reads MATLAB artifacts) ----------- + # The pythonArtifacts cases cannot run yet -- Python makes those in + # Stage 3 -- so they are deselected here rather than skipped (MATLAB + # reads them in Stage 4). Every case left must run: under + # NDR_SYMMETRY_CHECK_STRICT a skip fails. - name: "Stage 2: Python readArtifacts (reads MATLAB artifacts)" working-directory: NDR-python run: | - pytest tests/symmetry/read_artifacts/ -v --tb=short + pytest tests/symmetry/read_artifacts/ -v --tb=short -k "not pythonArtifacts" # -- Stage 3: Python makeArtifacts ------------------------------------ - name: "Stage 3: Python makeArtifacts" @@ -137,12 +172,36 @@ jobs: import matlab.unittest.TestRunner suite = TestSuite.fromPackage("ndr.symmetry.readArtifacts", "IncludingSubpackages", true); + + % KNOWN GAPS: MATLAB symmetry tests with no Python half, whose + % pythonArtifacts case therefore cannot run. Deselected by name and + % listed in the log, never left to come back Incomplete. Remove an + % entry when its Python half lands. + % readOmeZarr -- NDR-python has no OME-Zarr symmetry test yet. + noPythonHalf = ["readOmeZarr"]; + gap = contains({suite.Name}, "SourceType=pythonArtifacts") & ... + contains({suite.Name}, noPythonHalf); + for k = find(gap) + fprintf("Deselected (no Python half yet): %s\n", suite(k).Name); + end + suite = suite(~gap); + fprintf("\n=== MATLAB readArtifacts: %d test(s) ===\n", numel(suite)); assert(~isempty(suite), "No MATLAB readArtifacts tests found.") runner = TestRunner.withTextOutput("Verbosity", "Detailed"); results = runner.run(suite); - fprintf("\n=== MATLAB readArtifacts: %d passed, %d failed ===\n", ... - nnz([results.Passed]), nnz([results.Failed])); + fprintf("\n=== MATLAB readArtifacts: %d passed, %d failed, %d incomplete ===\n", ... + nnz([results.Passed]), nnz([results.Failed]), nnz([results.Incomplete])); assert(all(~[results.Failed]), "MATLAB readArtifacts failed") + + % Incomplete = the comparison did not run (the other suite's + % artifacts were missing). Strict mode: fail. + incomplete = results([results.Incomplete]); + for k = 1:numel(incomplete) + fprintf(2, "INCOMPLETE: %s\n", incomplete(k).Name); + end + assert(isempty(incomplete), ... + "%d MATLAB readArtifacts test(s) were Incomplete: their comparison did not run.", ... + numel(incomplete)) diff --git a/AGENTS.md b/AGENTS.md index 52c8213..cea7b73 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,7 +73,7 @@ commit hash in the command printed beneath it. Go there for: The bridge guard exists because *a check that could not run must not report the same result as a check that ran and passed*. The subtle part is that this applies to the guard's own tests, and it is easy to violate while writing -something careful. It has now happened three times in this repo: +something careful. It has now happened four times in this repo: 1. The `bridge` job named one test file, so a new one would have skipped in every job and read as green. @@ -84,6 +84,18 @@ something careful. It has now happened three times in this repo: actually discriminates — carried three `pytest.skip` calls gated on nothing. Had NDR-matlab's history made their fixture conditions true, the proof that the rule works would have stopped running, silently. +4. The cross-language **symmetry** job reported green on #33 with every + video comparison unrun (#34). Its NDR-matlab branch was pushed seconds + after the run had already fallen back to `main`, so there were no MATLAB + video artifacts. Python's read tests skipped, which was hidden among the + 16 skips every run carried by design, and MATLAB's read tests + `disp`+`return`ed, which `matlab.unittest` counts as **passed**. Now + `test-symmetry.yml` sets `NDR_SYMMETRY_CHECK_STRICT`: + `tests/symmetry/conftest.py` fails any skipped symmetry test (proved by + `tests/test_symmetry_strict.py`), the MATLAB stages fail on any + Incomplete result, the cases that cannot run in a stage by design are + deselected by name, and the job summary says which NDR-matlab branch was + used and why. None of these was careless in isolation; each was a reasonable local decision (*a test that cannot build its fixture should not fail*) that became a hole in @@ -93,7 +105,9 @@ CI. So the rule here is not "never skip" — it is: > `NDR_BRIDGE_CHECK_STRICT`, or it will hide.** In CI every bridge check is > expected to be *able* to run, so a skip there is a gap, not a courtesy. -`tests/conftest.py` now enforces this as a backstop: under +The same rule now holds for the symmetry tests under +`NDR_SYMMETRY_CHECK_STRICT` (entry 4). `tests/conftest.py` enforces it for the +bridge as a backstop: under `NDR_BRIDGE_CHECK_STRICT`, **any** skip in a `test_matlab_bridge_*.py` module becomes a failure — including one nobody thought to gate. It is derived rather than enumerated for the same reason the `needs_matlab` marker is: a list of diff --git a/tests/symmetry/conftest.py b/tests/symmetry/conftest.py index fff2f7f..a389e76 100644 --- a/tests/symmetry/conftest.py +++ b/tests/symmetry/conftest.py @@ -1,10 +1,17 @@ """Shared fixtures and configuration for NDR symmetry tests.""" +import os import tempfile from pathlib import Path +import pytest + from ndr.fun.ndrpath import ndrpath +#: Set by .github/workflows/test-symmetry.yml. Under it, a skipped symmetry +#: test FAILS; see :func:`pytest_runtest_makereport`. +STRICT_ENV_VAR = "NDR_SYMMETRY_CHECK_STRICT" + # Base directory where all symmetry artifacts live: # /NDR/symmetryTest///// SYMMETRY_BASE = Path(tempfile.gettempdir()) / "NDR" / "symmetryTest" @@ -50,3 +57,47 @@ def json_safe(value): if isinstance(value, float) and (math.isnan(value) or math.isinf(value)): return None return value + + +@pytest.hookimpl(wrapper=True, trylast=True) +def pytest_runtest_makereport(item, call): + """Under strict mode, a skipped symmetry test is a FAILED symmetry test. + + A read test skips when the other language produced no artifacts, and a + test of either kind skips when an example file or an optional dependency + is missing. Each of those is reasonable on a developer's machine, where + only one port may be installed. In CI, every comparison is expected to + run, so a skip there means the comparison did not happen while the job + still reported green. That is what hid the video symmetry tests on + VH-Lab/NDR-python#33's first run (VH-Lab/NDR-python#34). + + So, as in tests/conftest.py for the bridge tests, this is a backstop and + not a list: under strict mode ANY skip in a test under tests/symmetry/ + fails, including one nobody thought to gate. Cases that cannot run in a + given CI stage by design (the pythonArtifacts read cases in Stage 2, + before Python has made them) are deselected by that stage, not skipped. + """ + report = yield + + if not os.environ.get(STRICT_ENV_VAR, "").strip(): + return report + if not report.skipped or hasattr(report, "wasxfail"): + return report + if Path(__file__).parent not in Path(str(item.fspath)).parents: + return report + + reason = "" + if isinstance(report.longrepr, tuple) and len(report.longrepr) == 3: + reason = report.longrepr[2] + report.outcome = "failed" + report.longrepr = ( + f"{item.nodeid} was SKIPPED while {STRICT_ENV_VAR} is set.\n\n" + f" reason: {reason}\n\n" + "In CI every symmetry comparison is expected to run, so a skip here " + "reports the same result as a comparison that ran and matched. If the " + "other language's artifacts are missing, check that its half of the " + "test exists and that the workflow checked out the right branch of " + "the other repository (see the job summary). Do not delete the " + "comparison to make this green." + ) + return report diff --git a/tests/symmetry/read_artifacts/INSTRUCTIONS.md b/tests/symmetry/read_artifacts/INSTRUCTIONS.md index a454060..76a4b85 100644 --- a/tests/symmetry/read_artifacts/INSTRUCTIONS.md +++ b/tests/symmetry/read_artifacts/INSTRUCTIONS.md @@ -70,6 +70,10 @@ class TestReadData: ```bash # Verify artifacts (skips missing sources) pytest tests/symmetry/read_artifacts/ -v + +# As CI runs it: a skip FAILS, and the pythonArtifacts cases (made later, in +# Stage 3) are deselected rather than skipped +NDR_SYMMETRY_CHECK_STRICT=1 pytest tests/symmetry/read_artifacts/ -v -k "not pythonArtifacts" ``` ## Adding a new symmetry test: @@ -78,6 +82,12 @@ pytest tests/symmetry/read_artifacts/ -v `make_artifacts/` (e.g., `reader/`, `format/`). 2. Add a `test_.py` file with a parameterized test class that reads from both `matlabArtifacts` and `pythonArtifacts`. -3. Always use `pytest.skip()` when the expected artifact directory does not exist. +3. Use `pytest.skip()` when the expected artifact directory does not exist, so the suite + still runs on a machine with only one port installed. In CI that skip **fails**: + `test-symmetry.yml` sets `NDR_SYMMETRY_CHECK_STRICT`, under which + `tests/symmetry/conftest.py` turns any skip under `tests/symmetry/` into a failure, + because a comparison that did not run must not report green (VH-Lab/NDR-python#34). So a + new read test needs its MATLAB half to exist (on NDR-matlab `main`, or on a branch with + the same name, pushed before this one) or the symmetry job goes red. That is the point. 4. Mirror the MATLAB counterpart: `tools/tests/+ndr/+symmetry/+readArtifacts/+/.m`. diff --git a/tests/test_symmetry_strict.py b/tests/test_symmetry_strict.py new file mode 100644 index 0000000..0eaaf33 --- /dev/null +++ b/tests/test_symmetry_strict.py @@ -0,0 +1,55 @@ +"""Positive controls for the symmetry tests' strict mode (VH-Lab/NDR-python#34). + +``tests/symmetry/conftest.py`` turns any skipped symmetry test into a failure +when ``NDR_SYMMETRY_CHECK_STRICT`` is set, which is what keeps a comparison +that never ran from reporting green in ``test-symmetry.yml``. That guard is +only worth anything if it demonstrably fires, so these tests run one real +symmetry read test against an EMPTY artifact base, once without the flag (it +must skip) and once with it (it must fail). They live here rather than under +tests/symmetry/ because the default run ignores that directory, and a guard +that is only exercised by the job it guards is not exercised at all. +""" + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parents[1] +# Any symmetry read test will do; this one needs only the checked-in example +# videos and PyAV, both present wherever the full suite runs. +READ_TEST = "tests/symmetry/read_artifacts/reader/test_read_video.py" + +pytest.importorskip("av") + + +def _run(tmp_path, strict): + env = dict(os.environ) + # The symmetry artifacts live under tempfile.gettempdir(); an empty TMPDIR + # guarantees the other language's artifacts are absent. + env["TMPDIR"] = str(tmp_path) + env.pop("NDR_SYMMETRY_CHECK_STRICT", None) + if strict: + env["NDR_SYMMETRY_CHECK_STRICT"] = "1" + return subprocess.run( + [sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider", "-rfs", READ_TEST], + cwd=REPO, + env=env, + capture_output=True, + text=True, + ) + + +def test_missing_artifacts_skip_outside_strict_mode(tmp_path): + result = _run(tmp_path, strict=False) + assert result.returncode == 0, result.stdout + result.stderr + assert "skipped" in result.stdout and "failed" not in result.stdout, result.stdout + + +def test_missing_artifacts_fail_in_strict_mode(tmp_path): + result = _run(tmp_path, strict=True) + assert result.returncode != 0, result.stdout + result.stderr + assert "was SKIPPED while NDR_SYMMETRY_CHECK_STRICT is set" in result.stdout, result.stdout + assert " skipped" not in result.stdout.splitlines()[-1], result.stdout From c83c6a2c47589230a3f45c5e03bb316c6b45c67f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 14:54:29 +0000 Subject: [PATCH 2/2] symmetry CI: give matbox the runner's GITHUB_TOKEN Stage 1 of the symmetry job died before any test ran: matbox's installRequirements resolves NDR-matlab's MATLAB dependencies through the GitHub API unauthenticated, and hit the shared 60 req/hr limit ("GitHub API rate limit exceeded. Consider using a GITHUB_TOKEN environment variable."). NDR-matlab's own test-symmetry.yml already passes GITHUB_TOKEN: ${{ github.token }} in the job env for exactly this; port the same line here (built-in token, no secret to configure; 5000 req/hr). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01QjkvVXsdCVw6NNe5SnGNyX --- .github/workflows/test-symmetry.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/test-symmetry.yml b/.github/workflows/test-symmetry.yml index 27e496f..e2a3019 100644 --- a/.github/workflows/test-symmetry.yml +++ b/.github/workflows/test-symmetry.yml @@ -22,6 +22,13 @@ jobs: # on any Incomplete result. Cases that cannot run in a given stage by # design are deselected by name in that stage. VH-Lab/NDR-python#34. NDR_SYMMETRY_CHECK_STRICT: "1" + # matbox resolves and downloads its MATLAB dependencies (e.g. + # vhlab-toolbox-matlab) through the GitHub API. Give it the runner's + # built-in token (auto-provided; no secret to configure) so those calls + # are authenticated (5000 req/hr) instead of hitting the shared + # unauthenticated limit (60 req/hr), which fails dependency install. + # Same fix as NDR-matlab's test-symmetry.yml. + GITHUB_TOKEN: ${{ github.token }} steps: # -- Checkout both repos ----------------------------------------------