Skip to content

chore: harden the skip-value allocation test against unrelated allocations - #60

Merged
kinyoklion merged 1 commit into
v4from
rlamb/reader-allocs-test-hardening
Aug 13, 2026
Merged

chore: harden the skip-value allocation test against unrelated allocations#60
kinyoklion merged 1 commit into
v4from
rlamb/reader-allocs-test-hardening

Conversation

@kinyoklion

@kinyoklion kinyoklion commented Aug 13, 2026

Copy link
Copy Markdown
Member

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.


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.

Reviewed by Cursor Bugbot for commit 7fb41d9. Bugbot is set up for automated code reviews on this repo. Configure here.

@kinyoklion
kinyoklion marked this pull request as ready for review August 13, 2026 20:07
@kinyoklion
kinyoklion requested a review from a team as a code owner August 13, 2026 20:07
@kinyoklion
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
kinyoklion force-pushed the rlamb/reader-allocs-test-hardening branch from 71065bb to 7fb41d9 Compare August 13, 2026 20:14
@kinyoklion
kinyoklion marked this pull request as ready for review August 13, 2026 20:24
@kinyoklion
kinyoklion merged commit 904a44b into v4 Aug 13, 2026
13 checks passed
@kinyoklion
kinyoklion deleted the rlamb/reader-allocs-test-hardening branch August 13, 2026 20:32
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants