chore: harden the skip-value allocation test against unrelated allocations - #61
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:11
…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 easyjson build's expectation of exactly four allocations per parse is unchanged and holds under the averaging. The test body is otherwise unchanged.
kinyoklion
force-pushed
the
rlamb/reader-allocs-test-hardening-v3
branch
from
August 13, 2026 20:15
d3a3b23 to
3e2e4c7
Compare
kinyoklion
marked this pull request as ready for review
August 13, 2026 20:25
keelerm84
approved these changes
Aug 13, 2026
kinyoklion
added a commit
that referenced
this pull request
Aug 13, 2026
…tions (#60) `TestReaderSkipValueAllocations` failed once on the Windows / Go 1.25 / `-race` matrix 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.AllocsPerRun` samples the **process-global** malloc counter, and with `runs=1` the 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 `-race` Windows cell has the longest window in the matrix. The fix is one line: `AllocsPerRun(1, ...)` becomes `AllocsPerRun(100, ...)`. The function divides the total malloc delta by `runs` with 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=1` pattern (with an easyjson-conditional expectation) and gets the same fix in #61. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Hardens** `TestReaderSkipValueAllocations` so flaky CI failures (e.g. Windows / Go 1.25 / `-race`) are less likely when unrelated process allocations bump `testing.AllocsPerRun` with a single run. > > The measurement changes from **`AllocsPerRun(1, …)`** to **`AllocsPerRun(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. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7fb41d9. 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
2 participants
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.
v3 counterpart of #60 — the same one-line change:
AllocsPerRun(1, ...)becomesAllocsPerRun(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:
AllocsPerRunsamples the process-global malloc counter, soruns=1makes 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.
Note
Overview
Hardens
TestReaderSkipValueAllocationsso flaky CI failures from unrelated process allocations are less likely.The test still parses the same JSON and skips the nested
bobject while readingaandc, and still expects 0 allocs (or 4 under the easyjson build tag). The only behavioral change istesting.AllocsPerRun(1, …)→testing.AllocsPerRun(100, …), with comments explaining thatAllocsPerRunuses 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.Reviewed by Cursor Bugbot for commit 121ae6c. Bugbot is set up for automated code reviews on this repo. Configure here.