test(redaction): calibrate the perf growth sizes so the assertions cannot fail on a fast box - #22
Merged
Merged
Conversation
…nnot fail on a fast box CI caught a defect I shipped in #20: `the cookie rule stays linear as a single unbroken RUN grows` FAILED on the GitHub runner while the implementation was perfectly linear. Fixed at 8/16/32 KiB, a fast quiet runner finishes those sizes in a fraction of a millisecond, and at that scale timer resolution and JIT noise dominate the ratio. THAT IS THE SAME DEFECT CLASS THE ASSERTION WAS WRITTEN TO FIX, POINTING THE OTHER WAY. #20 replaced a perf test that could not FAIL with one that could not reliably PASS. A test that fails when nothing is wrong is exactly as useless as one that passes when something is, and it is worse in one respect: it trains people to re-run CI until it goes green, which is how a real failure gets waved through. The fix is to calibrate the smallest size to the machine rather than fix it, then measure across three doublings from there. The two cases separate themselves with no change to the 2.8 threshold, which is the part that must not move: - a LINEAR implementation is fast, so calibration keeps doubling and ends up measuring where the numbers are solid; - a QUADRATIC one is already far above the floor at the smallest size, so calibration stops immediately and the assertion fails at small sizes and therefore FAST. Applied to `growthPerDoubling` as well, because it is the same family and the same flake. The pre-existing Digest control failed 1 run in 8 here at loadavg 25, and an adversarial reviewer measured it at 2 in 10 before the estimator changed. VERIFIED IN BOTH DIRECTIONS, which is the whole point of the change: fix 10 of 10 consecutive full-suite runs green at loadavg 24.5 mutant ratio 4.012, FAILS, in 30.1s The mutant is `redactCookiePairs` replaced by the naive `/([^\s;,=]+)=([^\s;,]*)/g` that this rule was restructured to avoid. typecheck rc=0. Test-only change; `src/` is untouched. Refs: todos 6200c4e4 Agent: aemilius
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.
CI on the release PR (#21) caught a defect I shipped in #20, and it is the same defect class that PR was written to fix, pointing the other way.
the cookie rule stays linear as a single unbroken RUN growsfailed on the GitHub runner while the implementation is perfectly linear. Fixed at 8/16/32 KiB, a fast quiet runner finishes those sizes in a fraction of a millisecond, and at that scale timer resolution and JIT noise dominate the ratio.#20 replaced a perf test that could not FAIL with one that could not reliably PASS. A test that fails when nothing is wrong is exactly as useless as one that passes when something is — and worse in one respect: it trains people to re-run CI until it goes green, which is how a real failure gets waved through.
The fix
Calibrate the smallest size to the machine rather than fixing it, then measure across three doublings from there. The two cases separate themselves with no change to the 2.8 threshold, which is the part that must not move:
Applied to
growthPerDoublingas well, because it is the same family and the same flake: the pre-existing Digest control failed 1 run in 8 here at loadavg 25, and an adversarial reviewer measured it at 2 in 10 before the estimator changed.Verified in both directions
The mutant is
redactCookiePairsreplaced by the naive/([^\s;,=]+)=([^\s;,]*)/gthat the rule was restructured to avoid.typecheckrc=0.Test-only change —
src/is untouched, so no redaction behaviour moves.Why this is blocking something
@hasna/taion the registry is 0.1.4, which emits every session cookie it is asked to redact. Main carries the fixes for that (#19, #20) but is unpublished; the release PR #21 is blocked on this flake. Landing this unblocks the release.Refs: todos
6200c4e4Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.