Fail CI when a test fixture fails to generate#63
Merged
Conversation
Every fixture is produced by ffmpeg at configure time and the tests that use them are #ifdef-guarded, so a fixture that fails to generate removes test cases from the suite without failing anything. That is the same shape of silent gap as #55, where CI ran zero tests and stayed green: a pass that quietly means less than it appears to. CI installs all the dependencies these fixtures need, so on CI a disabled fixture is a real problem rather than graceful degradation. The configure output is now captured and the build fails if any fixture reports DISABLED, listing which one. The HDR fixture's message is reworded to the same "NAME fixture: enabled/DISABLED" form as the others so one grep covers all of them. Verified both directions: all three report enabled locally and on both CI platforms, and the guard does trip on a synthesised DISABLED line. Refs #58 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 18, 2026
kmatzen
added a commit
that referenced
this pull request
Jul 18, 2026
* Fail CI if a critical test case disappears Requiring the fixtures (#63) catches a fixture that fails to generate, but not a TEST_CASE deleted outright or gated on something other than a fixture. CI now checks by name for the cases covering bugs that reached main and were expensive to find: the seek contract on both the plain and B-frame fixtures, the EOF drain, position-counter drift, and seek past the end. Checking names rather than a total count was deliberate. A count needs bumping on every test addition, which trains people to update it reflexively -- including when it drops for a bad reason -- and it reports "the number changed" rather than which coverage was lost. Names fail with exactly what went missing and need no maintenance when tests are added. Verified in both directions: all five resolve against the current binary, and a deliberately bogus name is reported and fails the step. The whitespace strip matters -- the heredoc is indented for YAML, and without it no name would ever match and the check would pass vacuously, which is the same silent-success shape this is meant to prevent. Fixes #58 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Report which critical test cases were verified The check only printed on failure, so a pass that matched nothing looked exactly like a pass that matched everything -- the same silent-success shape the guard exists to catch, reintroduced in the guard itself. It now echoes each match and fails if it verified zero names. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 18, 2026
kmatzen
added a commit
that referenced
this pull request
Jul 19, 2026
) ctest reports pass/fail for the binary as a whole, and --output-on-failure hides Catch2's summary on success, so a green build recorded nothing about how many cases actually ran. A case that starts skipping stays present in --list-tests (so the #65 guard misses it) and needs no fixture (so the #63 guard misses it too), yet exercises nothing. That is not hypothetical: #70 was a completely broken FFV1 path hidden behind exactly this, for however long it had been there. Every codec these tests need is installed in CI, so a skip there is a real gap rather than graceful degradation, and the check fails the build. It uses the default console reporter deliberately -- --reporter compact omits the skip count entirely, so the grep would never fire. Verified both directions against real output: the line from a run with skips matches, the current clean run does not. Also bumps actions/checkout@v4 -> v5 and actions/setup-python@v5 -> v6 across all three workflows. Both target Node 20, which GitHub is currently force-running on Node 24 behind a shim it calls temporary; when that goes they break, for reasons unrelated to any change here. And adds __pycache__/ to .gitignore, which has been dirtying git status since the Python bindings landed. Fixes #66, fixes #68 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Addresses the remaining half of #58.
Every fixture is produced by
ffmpegat configure time, and the tests that use them are#ifdef-guarded. So a fixture that fails to generate silently removes test cases from the suite without failing anything — the same shape of gap as #55, where CI ran zero tests and stayed green. A pass that quietly means less than it appears to.This is not hypothetical for me: while verifying #51 the suite finished in 0.94s, and I had to go read the configure log to convince myself the new
[seek]cases had not been compiled out.Change
CI installs every dependency these fixtures need, so on CI a disabled fixture is a real problem rather than graceful degradation. The configure output is captured and the build fails if any fixture reports
DISABLED, naming it.The HDR fixture's message is reworded to the same
NAME fixture: enabled/DISABLEDform as the others, so one grep covers all three.Verified both directions
enabledlocally and on both CI platforms (checked the logs from Drain the decoder at EOF; cover B-frame reordering in seek tests #60's run before making them mandatory — I did not want to turn a graceful skip into a hard failure without knowing libx265 was actually present on both runners).DISABLEDline.What I did not do
#58 also suggested asserting a minimum test-case count. I skipped it: a hardcoded count needs bumping on every test addition, and it fails in a way that says "number changed" rather than "coverage was lost". Requiring the fixtures targets the actual failure mode directly. Happy to add the count too if you would rather have belt and braces.
🤖 Generated with Claude Code