Skip to content

Commit e7baff8

Browse files
Merge pull request #35 from VH-Lab/claude/clever-bardeen-hf5jgx
symmetry: a comparison that could not run fails CI instead of passing
2 parents 33b6418 + c83c6a2 commit e7baff8

5 files changed

Lines changed: 206 additions & 10 deletions

File tree

‎.github/workflows/test-symmetry.yml‎

Lines changed: 73 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,20 @@ jobs:
1515
symmetry:
1616
name: MATLAB <-> Python symmetry tests
1717
runs-on: ubuntu-latest
18+
env:
19+
# Every comparison is expected to run here, so a comparison that could
20+
# not run FAILS instead of reporting green: a skipped Python symmetry
21+
# test fails (tests/symmetry/conftest.py), and the MATLAB stages fail
22+
# on any Incomplete result. Cases that cannot run in a given stage by
23+
# design are deselected by name in that stage. VH-Lab/NDR-python#34.
24+
NDR_SYMMETRY_CHECK_STRICT: "1"
25+
# matbox resolves and downloads its MATLAB dependencies (e.g.
26+
# vhlab-toolbox-matlab) through the GitHub API. Give it the runner's
27+
# built-in token (auto-provided; no secret to configure) so those calls
28+
# are authenticated (5000 req/hr) instead of hitting the shared
29+
# unauthenticated limit (60 req/hr), which fails dependency install.
30+
# Same fix as NDR-matlab's test-symmetry.yml.
31+
GITHUB_TOKEN: ${{ github.token }}
1832

1933
steps:
2034
# -- Checkout both repos ----------------------------------------------
@@ -28,15 +42,29 @@ jobs:
2842
# Developers are expected to keep matching branch names across the
2943
# two repos when making cross-language changes. Falls back to main
3044
# when the matching branch does not exist in NDR-matlab.
45+
#
46+
# The choice is written to the job summary with its reason: a paired
47+
# change whose NDR-matlab branch was pushed AFTER this run started
48+
# resolves to main, and then fails in strict mode for want of the
49+
# MATLAB half -- push NDR-matlab first, or re-run once it exists.
3150
- name: Resolve NDR-matlab branch
3251
id: matlab-ref
3352
run: |
3453
BRANCH="${{ github.head_ref || github.ref_name }}"
3554
if git ls-remote --exit-code --heads https://github.com/VH-Lab/NDR-matlab.git "$BRANCH" >/dev/null 2>&1; then
36-
echo "ref=$BRANCH" >> "$GITHUB_OUTPUT"
55+
REF="$BRANCH"
56+
WHY="NDR-matlab has a branch named \`$BRANCH\`, matching this run."
3757
else
38-
echo "ref=main" >> "$GITHUB_OUTPUT"
58+
REF="main"
59+
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."
3960
fi
61+
echo "ref=$REF" >> "$GITHUB_OUTPUT"
62+
echo "NDR-matlab ref: $REF ($WHY)"
63+
{
64+
echo "### NDR-matlab checkout: \`$REF\`"
65+
echo
66+
echo "$WHY"
67+
} >> "$GITHUB_STEP_SUMMARY"
4068
4169
- name: Check out NDR-matlab (matching branch)
4270
uses: actions/checkout@v4
@@ -107,15 +135,29 @@ jobs:
107135
runner = TestRunner.withTextOutput("Verbosity", "Detailed");
108136
results = runner.run(suite);
109137
110-
fprintf("\n=== MATLAB makeArtifacts: %d passed, %d failed ===\n", ...
111-
nnz([results.Passed]), nnz([results.Failed]));
138+
fprintf("\n=== MATLAB makeArtifacts: %d passed, %d failed, %d incomplete ===\n", ...
139+
nnz([results.Passed]), nnz([results.Failed]), nnz([results.Incomplete]));
112140
assert(all(~[results.Failed]), "MATLAB makeArtifacts failed")
113141
142+
% Incomplete = an assumption failed (a missing example file or
143+
% tool), so these artifacts were never made. Strict mode: fail.
144+
incomplete = results([results.Incomplete]);
145+
for k = 1:numel(incomplete)
146+
fprintf(2, "INCOMPLETE: %s\n", incomplete(k).Name);
147+
end
148+
assert(isempty(incomplete), ...
149+
"%d MATLAB makeArtifacts test(s) were Incomplete: their artifacts were never made.", ...
150+
numel(incomplete))
151+
114152
# -- Stage 2: Python readArtifacts (reads MATLAB artifacts) -----------
153+
# The pythonArtifacts cases cannot run yet -- Python makes those in
154+
# Stage 3 -- so they are deselected here rather than skipped (MATLAB
155+
# reads them in Stage 4). Every case left must run: under
156+
# NDR_SYMMETRY_CHECK_STRICT a skip fails.
115157
- name: "Stage 2: Python readArtifacts (reads MATLAB artifacts)"
116158
working-directory: NDR-python
117159
run: |
118-
pytest tests/symmetry/read_artifacts/ -v --tb=short
160+
pytest tests/symmetry/read_artifacts/ -v --tb=short -k "not pythonArtifacts"
119161
120162
# -- Stage 3: Python makeArtifacts ------------------------------------
121163
- name: "Stage 3: Python makeArtifacts"
@@ -137,12 +179,36 @@ jobs:
137179
import matlab.unittest.TestRunner
138180
139181
suite = TestSuite.fromPackage("ndr.symmetry.readArtifacts", "IncludingSubpackages", true);
182+
183+
% KNOWN GAPS: MATLAB symmetry tests with no Python half, whose
184+
% pythonArtifacts case therefore cannot run. Deselected by name and
185+
% listed in the log, never left to come back Incomplete. Remove an
186+
% entry when its Python half lands.
187+
% readOmeZarr -- NDR-python has no OME-Zarr symmetry test yet.
188+
noPythonHalf = ["readOmeZarr"];
189+
gap = contains({suite.Name}, "SourceType=pythonArtifacts") & ...
190+
contains({suite.Name}, noPythonHalf);
191+
for k = find(gap)
192+
fprintf("Deselected (no Python half yet): %s\n", suite(k).Name);
193+
end
194+
suite = suite(~gap);
195+
140196
fprintf("\n=== MATLAB readArtifacts: %d test(s) ===\n", numel(suite));
141197
assert(~isempty(suite), "No MATLAB readArtifacts tests found.")
142198
143199
runner = TestRunner.withTextOutput("Verbosity", "Detailed");
144200
results = runner.run(suite);
145201
146-
fprintf("\n=== MATLAB readArtifacts: %d passed, %d failed ===\n", ...
147-
nnz([results.Passed]), nnz([results.Failed]));
202+
fprintf("\n=== MATLAB readArtifacts: %d passed, %d failed, %d incomplete ===\n", ...
203+
nnz([results.Passed]), nnz([results.Failed]), nnz([results.Incomplete]));
148204
assert(all(~[results.Failed]), "MATLAB readArtifacts failed")
205+
206+
% Incomplete = the comparison did not run (the other suite's
207+
% artifacts were missing). Strict mode: fail.
208+
incomplete = results([results.Incomplete]);
209+
for k = 1:numel(incomplete)
210+
fprintf(2, "INCOMPLETE: %s\n", incomplete(k).Name);
211+
end
212+
assert(isempty(incomplete), ...
213+
"%d MATLAB readArtifacts test(s) were Incomplete: their comparison did not run.", ...
214+
numel(incomplete))

‎AGENTS.md‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ commit hash in the command printed beneath it. Go there for:
7373
The bridge guard exists because *a check that could not run must not report
7474
the same result as a check that ran and passed*. The subtle part is that this
7575
applies to the guard's own tests, and it is easy to violate while writing
76-
something careful. It has now happened three times in this repo:
76+
something careful. It has now happened four times in this repo:
7777

7878
1. The `bridge` job named one test file, so a new one would have skipped in
7979
every job and read as green.
@@ -84,6 +84,18 @@ something careful. It has now happened three times in this repo:
8484
actually discriminates — carried three `pytest.skip` calls gated on
8585
nothing. Had NDR-matlab's history made their fixture conditions true,
8686
the proof that the rule works would have stopped running, silently.
87+
4. The cross-language **symmetry** job reported green on #33 with every
88+
video comparison unrun (#34). Its NDR-matlab branch was pushed seconds
89+
after the run had already fallen back to `main`, so there were no MATLAB
90+
video artifacts. Python's read tests skipped, which was hidden among the
91+
16 skips every run carried by design, and MATLAB's read tests
92+
`disp`+`return`ed, which `matlab.unittest` counts as **passed**. Now
93+
`test-symmetry.yml` sets `NDR_SYMMETRY_CHECK_STRICT`:
94+
`tests/symmetry/conftest.py` fails any skipped symmetry test (proved by
95+
`tests/test_symmetry_strict.py`), the MATLAB stages fail on any
96+
Incomplete result, the cases that cannot run in a stage by design are
97+
deselected by name, and the job summary says which NDR-matlab branch was
98+
used and why.
8799

88100
None of these was careless in isolation; each was a reasonable local decision
89101
(*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:
93105
> `NDR_BRIDGE_CHECK_STRICT`, or it will hide.** In CI every bridge check is
94106
> expected to be *able* to run, so a skip there is a gap, not a courtesy.
95107
96-
`tests/conftest.py` now enforces this as a backstop: under
108+
The same rule now holds for the symmetry tests under
109+
`NDR_SYMMETRY_CHECK_STRICT` (entry 4). `tests/conftest.py` enforces it for the
110+
bridge as a backstop: under
97111
`NDR_BRIDGE_CHECK_STRICT`, **any** skip in a `test_matlab_bridge_*.py` module
98112
becomes a failure — including one nobody thought to gate. It is derived rather
99113
than enumerated for the same reason the `needs_matlab` marker is: a list of

‎tests/symmetry/conftest.py‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,17 @@
11
"""Shared fixtures and configuration for NDR symmetry tests."""
22

3+
import os
34
import tempfile
45
from pathlib import Path
56

7+
import pytest
8+
69
from ndr.fun.ndrpath import ndrpath
710

11+
#: Set by .github/workflows/test-symmetry.yml. Under it, a skipped symmetry
12+
#: test FAILS; see :func:`pytest_runtest_makereport`.
13+
STRICT_ENV_VAR = "NDR_SYMMETRY_CHECK_STRICT"
14+
815
# Base directory where all symmetry artifacts live:
916
# <tempdir>/NDR/symmetryTest/<sourceType>/<namespace>/<class>/<test>/
1017
SYMMETRY_BASE = Path(tempfile.gettempdir()) / "NDR" / "symmetryTest"
@@ -50,3 +57,47 @@ def json_safe(value):
5057
if isinstance(value, float) and (math.isnan(value) or math.isinf(value)):
5158
return None
5259
return value
60+
61+
62+
@pytest.hookimpl(wrapper=True, trylast=True)
63+
def pytest_runtest_makereport(item, call):
64+
"""Under strict mode, a skipped symmetry test is a FAILED symmetry test.
65+
66+
A read test skips when the other language produced no artifacts, and a
67+
test of either kind skips when an example file or an optional dependency
68+
is missing. Each of those is reasonable on a developer's machine, where
69+
only one port may be installed. In CI, every comparison is expected to
70+
run, so a skip there means the comparison did not happen while the job
71+
still reported green. That is what hid the video symmetry tests on
72+
VH-Lab/NDR-python#33's first run (VH-Lab/NDR-python#34).
73+
74+
So, as in tests/conftest.py for the bridge tests, this is a backstop and
75+
not a list: under strict mode ANY skip in a test under tests/symmetry/
76+
fails, including one nobody thought to gate. Cases that cannot run in a
77+
given CI stage by design (the pythonArtifacts read cases in Stage 2,
78+
before Python has made them) are deselected by that stage, not skipped.
79+
"""
80+
report = yield
81+
82+
if not os.environ.get(STRICT_ENV_VAR, "").strip():
83+
return report
84+
if not report.skipped or hasattr(report, "wasxfail"):
85+
return report
86+
if Path(__file__).parent not in Path(str(item.fspath)).parents:
87+
return report
88+
89+
reason = ""
90+
if isinstance(report.longrepr, tuple) and len(report.longrepr) == 3:
91+
reason = report.longrepr[2]
92+
report.outcome = "failed"
93+
report.longrepr = (
94+
f"{item.nodeid} was SKIPPED while {STRICT_ENV_VAR} is set.\n\n"
95+
f" reason: {reason}\n\n"
96+
"In CI every symmetry comparison is expected to run, so a skip here "
97+
"reports the same result as a comparison that ran and matched. If the "
98+
"other language's artifacts are missing, check that its half of the "
99+
"test exists and that the workflow checked out the right branch of "
100+
"the other repository (see the job summary). Do not delete the "
101+
"comparison to make this green."
102+
)
103+
return report

‎tests/symmetry/read_artifacts/INSTRUCTIONS.md‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,10 @@ class TestReadData:
7070
```bash
7171
# Verify artifacts (skips missing sources)
7272
pytest tests/symmetry/read_artifacts/ -v
73+
74+
# As CI runs it: a skip FAILS, and the pythonArtifacts cases (made later, in
75+
# Stage 3) are deselected rather than skipped
76+
NDR_SYMMETRY_CHECK_STRICT=1 pytest tests/symmetry/read_artifacts/ -v -k "not pythonArtifacts"
7377
```
7478

7579
## Adding a new symmetry test:
@@ -78,6 +82,12 @@ pytest tests/symmetry/read_artifacts/ -v
7882
`make_artifacts/` (e.g., `reader/`, `format/`).
7983
2. Add a `test_<name>.py` file with a parameterized test class that reads from both
8084
`matlabArtifacts` and `pythonArtifacts`.
81-
3. Always use `pytest.skip()` when the expected artifact directory does not exist.
85+
3. Use `pytest.skip()` when the expected artifact directory does not exist, so the suite
86+
still runs on a machine with only one port installed. In CI that skip **fails**:
87+
`test-symmetry.yml` sets `NDR_SYMMETRY_CHECK_STRICT`, under which
88+
`tests/symmetry/conftest.py` turns any skip under `tests/symmetry/` into a failure,
89+
because a comparison that did not run must not report green (VH-Lab/NDR-python#34). So a
90+
new read test needs its MATLAB half to exist (on NDR-matlab `main`, or on a branch with
91+
the same name, pushed before this one) or the symmetry job goes red. That is the point.
8292
4. Mirror the MATLAB counterpart:
8393
`tools/tests/+ndr/+symmetry/+readArtifacts/+<namespace>/<ClassName>.m`.

‎tests/test_symmetry_strict.py‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
"""Positive controls for the symmetry tests' strict mode (VH-Lab/NDR-python#34).
2+
3+
``tests/symmetry/conftest.py`` turns any skipped symmetry test into a failure
4+
when ``NDR_SYMMETRY_CHECK_STRICT`` is set, which is what keeps a comparison
5+
that never ran from reporting green in ``test-symmetry.yml``. That guard is
6+
only worth anything if it demonstrably fires, so these tests run one real
7+
symmetry read test against an EMPTY artifact base, once without the flag (it
8+
must skip) and once with it (it must fail). They live here rather than under
9+
tests/symmetry/ because the default run ignores that directory, and a guard
10+
that is only exercised by the job it guards is not exercised at all.
11+
"""
12+
13+
import os
14+
import subprocess
15+
import sys
16+
from pathlib import Path
17+
18+
import pytest
19+
20+
REPO = Path(__file__).resolve().parents[1]
21+
# Any symmetry read test will do; this one needs only the checked-in example
22+
# videos and PyAV, both present wherever the full suite runs.
23+
READ_TEST = "tests/symmetry/read_artifacts/reader/test_read_video.py"
24+
25+
pytest.importorskip("av")
26+
27+
28+
def _run(tmp_path, strict):
29+
env = dict(os.environ)
30+
# The symmetry artifacts live under tempfile.gettempdir(); an empty TMPDIR
31+
# guarantees the other language's artifacts are absent.
32+
env["TMPDIR"] = str(tmp_path)
33+
env.pop("NDR_SYMMETRY_CHECK_STRICT", None)
34+
if strict:
35+
env["NDR_SYMMETRY_CHECK_STRICT"] = "1"
36+
return subprocess.run(
37+
[sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider", "-rfs", READ_TEST],
38+
cwd=REPO,
39+
env=env,
40+
capture_output=True,
41+
text=True,
42+
)
43+
44+
45+
def test_missing_artifacts_skip_outside_strict_mode(tmp_path):
46+
result = _run(tmp_path, strict=False)
47+
assert result.returncode == 0, result.stdout + result.stderr
48+
assert "skipped" in result.stdout and "failed" not in result.stdout, result.stdout
49+
50+
51+
def test_missing_artifacts_fail_in_strict_mode(tmp_path):
52+
result = _run(tmp_path, strict=True)
53+
assert result.returncode != 0, result.stdout + result.stderr
54+
assert "was SKIPPED while NDR_SYMMETRY_CHECK_STRICT is set" in result.stdout, result.stdout
55+
assert " skipped" not in result.stdout.splitlines()[-1], result.stdout

0 commit comments

Comments
 (0)