Repository navigation
test(immune): a missing bng_cpp must fail this file, not skip it - #72
Merged
Merged
Conversation
Every test in this file checks bng_cpp's output, so bng_cpp is the subject,
not an optional dependency. The file nonetheless gated all 22 tests on
requires_bng_cpp = pytest.mark.skipif(_bng_cpp() is None, ...)
and _require_bng_cpp() called pytest.skip() on the same condition. With the
binary absent the file reported success having executed nothing: 0 passed, 0
failed, 22 skipped, exit 0. From CI that is indistinguishable from a green run.
Correctness found this while reviewing the merged PR and flagged it as a third
silent-pass mechanism keyed on the CLI binary rather than the extension; they
correctly declined to convert it because the file is mine. It is mine, so this
is the conversion.
CHANGED
- _require_bng_cpp() now raises AssertionError naming the path it looked for,
instead of skipping.
- The skipif decorator and all 22 @requires_bng_cpp uses are removed; they
were the mechanism and are now unreachable.
- run_model() takes the strict helper rather than asserting on an Optional.
MEASURED, both directions, one command each:
BNG_CPP=build/cpp/bng_cpp -> 22 passed in 0.65s, 0 skipped
BNG_CPP=/nonexistent/bng_cpp -> 22 failed in 0.41s, 0 skipped
Before this commit the second command reported 22 skipped and exit 0. The
count 22 is identical either way, which is precisely why the skip count has to
be compared against expectation rather than trusted.
BNG2 KEEPS ITS REAL SKIP. The two oracle-parity tests still skip when
BNG2.pl is absent, because that is a genuinely optional external oracle rather
than the subject under test. They do still require bng_cpp, since they compare
its output against the oracle's.
Not affected: the 22 assertions, the models, and the observable defect pinning
they carry. This changes only what happens when the binary is missing, from
quiet success to a named failure.
ruff clean; black clean (--target-version py39).
akutuva21
force-pushed
the
agent/sci-immuno
branch
from
October 1, 2026 03:21
df4681c to
23ca38d
Compare
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.
Follow-up to #68, which is merged. Found by @correctness while reviewing it; the file is mine, so this is the fix.
The defect
Every test in
tests/python/test_immune_models.pychecks bng_cpp's output, so bng_cpp is the subject under test, not an optional dependency. The file nonetheless gated all 22 tests on a skip:and
_require_bng_cpp()calledpytest.skip()on the same condition. With the binary absent the file reported success having executed nothing: 0 passed, 0 failed, 22 skipped, exit 0. From CI that is indistinguishable from a green run.Measured, both directions, one command each
Before this commit the second command reported 22 skipped and exit 0. The count of 22 is identical either way, which is exactly why a skip count has to be compared against expectation rather than trusted.
The change
_require_bng_cpp()raisesAssertionErrornaming the path it looked for, instead of skipping.skipifdecorator and all 22@requires_bng_cppuses are removed — they were the mechanism and are now unreachable.run_model()takes the strict helper rather than asserting on anOptional.BNG2 keeps its real skip
The two oracle-parity tests still skip when
BNG2.plis absent. That is a genuinely optional external oracle, not the subject under test. They do still require bng_cpp, since they compare its output against the oracle's.Not affected
The 22 assertions, the five models, and the observable defect pinning they carry. This changes only what happens when the binary is missing: from quiet success to a named failure.
One file, +20/-24. No C++, no CMake. ruff clean; black clean (
--target-version py39).🤖 Generated with Claude Code