test: reconcile ent#89 tests with ent#128's compatibility contract - #1957
Conversation
#1899 (ent#128) and #1946 (ent#89) were both green in isolation and both merged to dev, but four ent#89 tests assert against pre-#1899 API and behaviour, so the INTEGRATION is red on dev. Nothing in the production code is wrong; the assertions describe a contract that #1899 deliberately changed. Caught by the regression-diff job, which runs after the four required checks and therefore after auto-merge had already landed #1946 (dev's required set is Analyze x2 + schema-parity + verify-non-root; it runs no unit tests). tests/unit/test_compatibility_checks.py test_a_raising_check_is_logged run_static's swallow now returns _fail(..., {"check_error": ...}), not _skip(..., "check_error"), so there is no skip_reason to read. Asserts the FAIL shape instead -- which is the point of the change, since only a counted status reaches hard_count/soft_count. A sibling in the same file already asserted "fail" and was passing, so the file was internally inconsistent. test_persisted_check_error_does_not_replay_as_clean Asserted soft_count == 0 for a persisted skipped+check_error row -- i.e. it pinned the BUG as expected behaviour. #1899's _did_not_pass counts that row as a finding on purpose, so the correct expectation is 1. Kept as a real regression test: a row persisted by an OLDER build, before the swallow started returning fail, still must not replay as clean. tests/unit/test_ent89_template_schedules.py (x2) _build_local_template gained a required keyword-only is_bundled parameter in #1899 (it selects the credential-metadata trust label). Both call sites updated; orthogonal to the schedules: block under test. Verified: test_compatibility_checks.py + test_ent89_template_schedules.py + test_ent128b1_compat_gates.py -> 225 passed, 1 skipped. Full tests/unit on this branch -> 6698 passed (the four hypothesis property files need a dependency absent from this host and were excluded).
vybe
left a comment
There was a problem hiding this comment.
Approving my own finding, with the reasoning stated so it can be checked rather than taken on trust.
This is a test-only reconciliation, not a behaviour change. The diff is two test files, +18/-7, no production code. Each of the four assertions was correct when written and became wrong when #1899 landed:
test_a_raising_check_is_loggedreaddetail["skip_reason"], which stopped existing when the swallow moved from_skipto_fail. Asserting the FAIL shape is not a workaround — it is the property #1899 added, since only a counted status reacheshard_count/soft_count. The same file already had a passing sibling asserting"fail", so it was internally inconsistent.test_persisted_check_error_does_not_replay_as_cleanassertedsoft_count == 0for a persistedskipped+check_errorrow — it pinned the bug as expected behaviour._did_not_passcounts that row deliberately. Flipped to1and kept with teeth: a row persisted by an older build still must not replay as clean.
3/4._build_local_templategained a required keyword-onlyis_bundledin #1899; both ent#89 call sites now pass it.
The process gap is the real finding. dev's four required checks run no unit tests, so auto-merge landed #1946 before regression diff reported. Two independently-green PRs can therefore redden dev on contact, with nothing blocking either. Worth considering regression diff as a required check — filing that separately rather than in this PR.
Verified: the three affected files → 225 passed / 1 skipped; full tests/unit on this branch → 6698 passed. The two unrelated files that misbehaved in my local whole-suite run pass on a pristine origin/dev worktree and pass in CI on both base and head, so they are local ordering artifacts, not regressions.
What
Four unit tests are failing on
devright now. This fixes them.They are not a defect in either PR that produced them: #1899 (ent#128) and #1946 (ent#89) were each green in isolation, and the failures only exist in their combination. Nothing in the production code is wrong — the four assertions describe a contract that #1899 deliberately changed, and #1946 was written before that landed.
Why it reached
devdev's required checks areAnalyze (python),Analyze (javascript-typescript),schema-parityandverify-non-root. None of them runs a unit test. Theregression diffjob does, but it reports after the required set goes green — so auto-merge had already landed #1946 by the time the failures were visible.regression diffon the merge commit reported exactly these four, against a base with zero failures:All six
pytestjobs (base and head × 3 seeds) themselves passed — they emit JUnit XML and letregression diffbe the gate.The four
1.
test_a_raising_check_is_loggedrun_static's per-check swallow used to record_skip(..., "check_error"). #1899 changed it to_fail(..., {"check_error": ...})so that a crashed check is actually counted by_counts— without that, a raise inside a HARD check droppedhard_countand flippedoverall_statustocompatibleon a broken agent.The test still read
detail["skip_reason"], which no longer exists. It now asserts the FAIL shape, which is the fix — only a counted status reacheshard_count/soft_count.Worth noting the file was already internally inconsistent: a sibling test in the same class asserts
t018["status"] == "fail"and was passing.2.
test_persisted_check_error_does_not_replay_as_cleanThis asserted
_counts(...)["soft_count"] == 0for a persistedskipped+check_errorrow — i.e. it pinned the bug as expected behaviour. #1899's_did_not_passcounts exactly that row as a finding, on purpose.Expectation flipped to
1, and the docstring rewritten so the test keeps real teeth: a row persisted by an older build, before the swallow started returningfail, still must not replay as a clean bill of health on a stopped-agent read.3 & 4.
_build_local_template(...)#1899 gave it a required keyword-only
is_bundledparameter (it selects the credential-metadata trust label, so an operator-uploaded template does not inherit the bundled catalog's label). Both ent#89 call sites now pass it. Orthogonal to theschedules:block under test.Verification
Full backend unit suite on this branch:
Two unrelated files (
test_1081_physical_meter.py,test_subscription_auto_switch_pingpong.py) reported failures/errors in that local whole-suite run but pass on a pristineorigin/devworktree and pass in CI on both base and head — local suite-ordering artifacts on this host, not touched by this change (the diff is two test files). The fourtest_1771*hypothesis property files were excluded: the dependency is absent from this host.Scope
Two test files, 18 insertions / 7 deletions. No production code.