Run the python suite in CI as a ratchet - #166
Merged
Merged
Conversation
The python suite is not run by CI at all -- .github/workflows has rust-ci,
oss-readiness, governance and auto-approve, and none of them invoke it. That is
how nine test files which could not even import sat here unnoticed: every test in
them errored on import, and nothing reported it.
Making the suite a blocking gate today would mean fixing 131 pre-existing
failures before anything else could land. This records what fails now and fails
only when a change ADDS a failure. Existing failures stay visible, and the
baseline shrinks as they are fixed -- rerun with --record to bank the wins.
## Why a second full run instead of re-running the new names
Several tests here fail in one run and pass in the next. A ratchet that trips on
those is a ratchet nobody keeps green, so a new failure has to appear in two full
runs before the build fails.
The cheaper-looking option does not work: filtering with `-k` still imports every
module, so unrelated import errors appear in the output and there is no reliable
way to tell "this test failed" from "the suite could not load". Measured -- an
isolated run of a PASSING test reported 8 errors. Intersecting two full runs needs
no such judgement, and the second pass only runs when something new showed up.
## Two bugs found while building it
The first cut returned `bool(generator)` from the isolation check. A generator
object is always truthy, so every new failure would have been treated as real and
the retry silently defeated -- the check would have looked like it worked.
The second counted any failure line in the isolated output rather than lines
naming the target test, which made it always true for the reason above.
Both are recorded because they share a shape: a guard that cannot fail is
indistinguishable from a guard that passes.
Verified on a clean tree ("no new failures") and against a deliberately injected
failing test.
… uvicorn The first CI run of this job failed, which was the job doing its work on itself: the baseline had been recorded on a developer machine, and CI sees a different set. CI reported 9 tests passing that fail locally -- the browser-facing ones need a newer node than this machine has -- and 3 failing that pass locally, which need packages CI does not install. The baseline now records what the RUNNER sees, and the script says so, because a local run will always differ in both directions. The fourth name CI flagged was mine: test_mem0_read_your_writes imports uvicorn to serve the real gateway, and CI installs python only, so it errored. It now skips when uvicorn is absent. A test that cannot run should say so rather than report as a failure -- this suite already has enough failures that are really something else.
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
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.
The python suite is not run by CI.
.github/workflows/has rust-ci, oss-readiness, governanceand auto-approve, and none of them invoke it.
That is how nine test files which could not even import sat here unnoticed — every test in them
errored on import, and nothing reported it. (Removed in #149.)
Why a ratchet rather than a gate
Making the suite blocking today would mean fixing 131 pre-existing failures before anything
else could land. This records what fails now in
tools/python_suite_baseline.jsonand fails onlywhen a change adds a failure. Existing failures stay visible, and the baseline shrinks as
they are fixed — rerun with
--recordto bank the wins. The job also prints what got fixed, soprogress is visible without anyone hunting for it.
Why it re-runs the whole suite to confirm
Several tests here fail in one run and pass in the next — order-dependent, or racing background
work. A ratchet that trips on those is a ratchet nobody keeps green, so a new failure must appear
in two full runs before the build fails. The second pass only runs when something new showed
up.
The cheaper-looking option does not work. Filtering with
-kstill imports every module, sounrelated import errors land in the output and there is no reliable way to tell "this test
failed" from "the suite could not load". Measured: an isolated run of a passing test
reported 8 errors.
Verified
The canary run also shows the flake filtering working: the first version of this script reported
test_background_worker_refreshes_dirty_nodes_and_embeddingsalongside the real failure, and thetwo-pass version reports only the real one.
Two bugs worth naming
The first cut returned
bool(generator)from its isolation check. A generator object is alwaystruthy, so every new failure would have been treated as real and the flake filtering silently
defeated — the check would have looked like it worked. The second counted any failure line
rather than lines naming the target test, which was always true for the reason above.
Both share a shape worth watching for: a guard that cannot fail is indistinguishable from a
guard that passes. That is also the argument for the canary test above — a ratchet nobody has
seen fail is not known to work.