diff --git a/.github/workflows/test-symmetry.yml b/.github/workflows/test-symmetry.yml index cdc1c79..e2a3019 100644 --- a/.github/workflows/test-symmetry.yml +++ b/.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)) 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