symmetry: a comparison that could not run fails CI instead of passing

.github/workflows/test-symmetry.yml

@@ -15,6 +15,20 @@ 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"
+      # 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 ----------------------------------------------
@@ -28,15 +42,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 +135,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 +179,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))

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

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:
 #   <tempdir>/NDR/symmetryTest/<sourceType>/<namespace>/<class>/<test>/
 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

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_<name>.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/+<namespace>/<ClassName>.m`.

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