test(index): assert the scan-budget ordering with a gate instead of a stopwatch - #362
Merged
Merged
Conversation
… stopwatch The test means "the read returns while the scan is still going". It asserted that by racing a 400ms sleep against a 300ms wall-clock ceiling, which is a question about how fast the machine is, not about what the code did. On CI it came up 320ms on ubuntu while macos and windows passed, and a re-run went green. The scraper now blocks on a promise only the test can resolve, so the scan provably has not finished when the assertions run — nothing but the test can finish it. The wall clock is gone from the assertions entirely. Two things the old version could not say and this one does: that the scan had actually started (otherwise an empty answer and `isScanning()` would also hold for a scan that never began), and that the empty answer means "not yet" rather than "never" — the gate opens, the scan settles, and the session is there. Honest about the evidence: I could not reproduce the failure locally. Eight runs before the fix and ten after passed, and ten runs of the *old* test under eight saturating CPU loops also passed, so the load I could generate is not the condition CI hit. The justification is the observed failure itself — 320ms against a 300ms bound is a timing assertion losing a race — plus the fact that the new assertions cannot fail that way whatever the cause was. The two other SlowScraper tests are left alone. They assert on settled state rather than on elapsed time, so neither races anything.
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.
The test means "the read returns while the scan is still going". It asserted that by racing a 400ms sleep against a 300ms wall-clock ceiling — a question about how fast the machine is, not about what the code did.
On #361 it came up 320ms on ubuntu while macos and windows passed. A re-run went green. That's the habit this fixes: a test that fails on machine speed teaches everyone to press re-run, which is how a real intermittent failure gets waved through.
The change
The scraper now blocks on a promise only the test can resolve. The scan provably hasn't finished when the assertions run, because nothing but the test can finish it. No wall clock in any assertion.
Two things the old version couldn't say and this one does:
isScanning() === truewould otherwise also hold for a scan that never beganThe gate is also released in a
finally, so a failed assertion can't hang the suite on a gate nobody opened (close()waits for the scan to settle).Honest about the evidence
I could not reproduce the failure locally. Eight runs before the fix and ten after passed; ten runs of the old test under eight saturating CPU loops also passed. The load I could generate is not the condition CI hit.
The justification is the observed failure itself — 320ms against a 300ms bound is a timing assertion losing a race — plus the fact that the new assertions cannot fail that way whatever the cause was.
Scope
The two other
SlowScrapertests are left alone. They assert on settled state rather than elapsed time, so neither races anything.706 tests pass, typecheck and lint clean.