Skip to content

ci: fix the failures in the new test-race job - #4228

Open
aheritier wants to merge 4 commits into
mainfrom
fix/test-race-failures
Open

ci: fix the failures in the new test-race job#4228
aheritier wants to merge 4 commits into
mainfrom
fix/test-race-failures

Conversation

@aheritier

@aheritier aheritier commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Why

#4221 added a test-race job (go test -race -shuffle=on ./...) on pushes to main. Its first run ( 34493297831 ) failed while test-linux passed. Nothing in the merged code regressed: the race detector exposed pre-existing bugs that go test never saw. Two packages failed for four root causes; the ~15 other "race detected during execution of test" failures are collateral, since Go fails every test running when a race fires.

What

Commit Root cause Kind
fix(evaluation) runDockerAgentInContainer read stderrData after cmd.Wait() while its own goroutine was still writing it; Wait does not wait for our goroutine. Stderr now goes through cmd.Stderr = &bytes.Buffer{}, which Wait (bounded by WaitDelay) synchronizes. Production race
test(evaluation) withVersion mutated the global version.Version from a t.Parallel() test while sibling parallel tests read it via DefaultAgentImage(). Extracted a pure defaultAgentImageFor(v) and table-test that instead; nothing writes version.Version from tests any more. Test-only race
test(tui) freeze clock TestActualProgramScrollCancelResizeMatrixNeverNeedsRecoveryClick compares two frames taken milliseconds apart; under -race a spinner TickMsg lands between them and advances the glyph. New frozenClockRoot builds the harness on a scheduler that never schedules a tick, used by the three exact-frame tests. Perf/geometry tests stay on wallClockRoot. Flaky assertion
test(tui) teardown When that assertion failed, require aborted before program.Quit(), leaking a live render loop whose View() read styles globals while later tests called styles.ApplyTheme. New startTestProgram helper quits, waits (bounded) and stops the runtime in t.Cleanup; all eight inline program starts use it. Test hygiene

Verification

  • go test -race -shuffle=on -count=5 ./pkg/evaluation: clean.
  • go test -race -count=20 -run 'TestActualProgram|TestColorScheme|TestHandleChangeTheme' ./pkg/tui: clean.
  • Forcing the matrix assertion to fail now fails only that test; the theme tests no longer race.
  • task lint passes.

Not in this PR

A shuffled full pkg/tui race run still fails ~1 in 5 on TestLoadSessionThenClickEditLabel, from an unrelated production race in pkg/app (App.Start's goroutine reads a.session while ReplaceSession writes it). That one was not in run 34493297831 and is tracked and fixed separately.

…ta race

runDockerAgentInContainer read stderrData after cmd.Wait() while a
hand-rolled goroutine was still writing it; Wait does not wait for our
own goroutine. Let exec.Cmd own the copy via a bytes.Buffer so Wait
(bounded by WaitDelay) guarantees the data is complete before we read it.
withVersion wrote the global version.Version from a t.Parallel() test
while sibling parallel tests read it through DefaultAgentImage(), which
the race detector flagged. Extract the pure defaultAgentImageFor helper
and table-test it with explicit versions instead.
Eight tests started a program inline and only quit it at the end of the
function, so a failed require leaked a live render loop whose View()
kept reading styles globals while later tests called ApplyTheme. The
race detector then failed unrelated theme tests. startTestProgram now
guarantees Quit, a bounded wait for Run, and ar.Stop() in t.Cleanup.
Tests comparing two frames across a message round trip flaked when a
spinner TickMsg landed between them and advanced the glyph; -race slows
the loop enough to straddle TickRate. frozenClockRoot builds the harness
on a scheduler that never schedules a tick, so ar.Now() stays constant.
Perf and geometry tests stay on wallClockRoot.
@aheritier
aheritier requested a review from a team as a code owner September 10, 2026 16:24
@aheritier
aheritier enabled auto-merge September 10, 2026 16:29
@aheritier aheritier added area/tui For features/issues/fixes related to the TUI status/needs-triage For issues that need to be triaged kind/test Test-only changes labels Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/tui For features/issues/fixes related to the TUI kind/test Test-only changes status/needs-triage For issues that need to be triaged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant