chore: harden the skip-value allocation test against unrelated allocations - #60
Merged
Merged
Conversation
kinyoklion
marked this pull request as ready for review
August 13, 2026 20:07
kinyoklion
marked this pull request as draft
August 13, 2026 20:10
…cations AllocsPerRun samples a process-wide allocation counter, so with runs=1 a single unrelated allocation on another goroutine (a timer, a finalizer) during the measured window fails the test. Average over 100 runs instead: the integer division absorbs stray allocations, while an allocation in the code under test still counts once per run and fails the test every time. The test body is otherwise unchanged.
kinyoklion
force-pushed
the
rlamb/reader-allocs-test-hardening
branch
from
August 13, 2026 20:14
71065bb to
7fb41d9
Compare
kinyoklion
marked this pull request as ready for review
August 13, 2026 20:24
keelerm84
approved these changes
Aug 13, 2026
kinyoklion
added a commit
that referenced
this pull request
Aug 13, 2026
…tions (#61) v3 counterpart of #60 — the same one-line change: `AllocsPerRun(1, ...)` becomes `AllocsPerRun(100, ...)`, with the test body otherwise unchanged. The easyjson-conditional expectation is preserved: that build's exactly-4-allocations-per-parse count is deterministic and holds under the averaging (the integer division yields 400/100 = 4). Verified under both build tags, including `-race`. See #60 for the analysis: `AllocsPerRun` samples the process-global malloc counter, so `runs=1` makes the assertion "nothing anywhere in the process allocates during the window" — one stray timer or finalizer allocation on a slow runner reads as a failure. Averaging over 100 runs absorbs strays through the integer division, while a genuine allocation in the code under test occurs in every run and still fails. No interaction with the open backport PRs (#56/#57/#58) — none of them touch this file. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Hardens `TestReaderSkipValueAllocations`** so flaky CI failures from unrelated process allocations are less likely. > > The test still parses the same JSON and skips the nested `b` object while reading `a` and `c`, and still expects **0** allocs (or **4** under the easyjson build tag). The only behavioral change is **`testing.AllocsPerRun(1, …)` → `testing.AllocsPerRun(100, …)`**, with comments explaining that `AllocsPerRun` uses a process-wide counter, so a single run can fail if another goroutine allocates; averaging 100 runs smooths stray timer/finalizer noise while real per-run allocs in the code under test still fail the assertion. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 121ae6c. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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.
TestReaderSkipValueAllocationsfailed once on the Windows / Go 1.25 /-racematrix cell (expected 0 allocations, measured 1) and passed on re-run. The test is sound about the reader — the fragility is in the measurement:testing.AllocsPerRunsamples the process-global malloc counter, and withruns=1the assertion is effectively "nothing anywhere in the process allocates during this window."GOMAXPROCS(1)serializes goroutines but does not stop them, so one timer fire, finalizer, or stray wakeup during the window reads as a failure — and the-raceWindows cell has the longest window in the matrix.The fix is one line:
AllocsPerRun(1, ...)becomesAllocsPerRun(100, ...). The function divides the total malloc delta byrunswith integer division, so up to 99 stray allocations across the window read as 0, while a genuine allocation in the code under test occurs in every run and still reads as ≥ 1. The test body is otherwise unchanged.Verified: 20 consecutive runs green plus 5 under
-race; with a deliberate allocation injected into the closure, the test still fails every time.The v3 line's copy of this test has the same
runs=1pattern (with an easyjson-conditional expectation) and gets the same fix in #61.Note
Overview
Hardens
TestReaderSkipValueAllocationsso flaky CI failures (e.g. Windows / Go 1.25 /-race) are less likely when unrelated process allocations bumptesting.AllocsPerRunwith a single run.The measurement changes from
AllocsPerRun(1, …)toAllocsPerRun(100, …), with comments explaining that the counter is process-global and integer-averaging tolerates sporadic timer/finalizer noise while still reporting ≥1 when the closure allocates every iteration. The skip-value exercise and 0 expected allocation assertion are unchanged.Reviewed by Cursor Bugbot for commit 7fb41d9. Bugbot is set up for automated code reviews on this repo. Configure here.