Skip to content

chore: Fix flaky tests with deterministic runs#4377

Merged
gmlewis merged 1 commit into
google:masterfrom
gmlewis:use-synctest
Jul 10, 2026
Merged

chore: Fix flaky tests with deterministic runs#4377
gmlewis merged 1 commit into
google:masterfrom
gmlewis:use-synctest

Conversation

@gmlewis

@gmlewis gmlewis commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

This PR addresses a recent race failure on Windows demonstrated here:
https://github.com/gmlewis/go-github/actions/runs/29060026853/job/86259654619

The crash was fatal error: bad g->status in ready — a Go runtime bug on Windows + -race, triggered when parallel tests with goroutines/timers complete under heavy load.

Root cause of flakiness: Two rate-limit tests relied on wall-clock timing:

  • abortSleepContextCancelled: spawned a goroutine running client.Do + used time.After(10s) timeouts (2 real timers active)
  • abortSleepContextCancelledClientLimit: used context.WithTimeout(10ms) — a tiny window racing with goroutine scheduling

Fix — made both fully deterministic with zero real-time dependencies:

  1. abortSleepContextCancelled: Removed the errCh goroutine + time.After timeouts. Now calls client.Do synchronously; a tiny goroutine just waits for the handler's requestReceived signal then calls cancel(). The HTTP transport is synchronous, so the handler always completes before the rate-limit sleep begins — cancel fires either just before or during the sleep, both handled identically by sleepUntilResetWithBuffer's select.
  2. abortSleepContextCancelledClientLimit: Replaced WithTimeout(10ms) with a pre-cancelled context (WithCancel + immediate cancel()). checkRateLimitBeforeDo → sleepUntilResetWithBuffer sees ctx.Done() already closed and returns instantly — no timing race at all.

Also fixed a copy-paste bug: "Expected 1 requests" → "Expected 0 requests".

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jul 10, 2026
@gmlewis

gmlewis commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.51%. Comparing base (9b2c665) to head (9d464e1).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4377   +/-   ##
=======================================
  Coverage   97.51%   97.51%           
=======================================
  Files         193      193           
  Lines       19526    19526           
=======================================
  Hits        19040    19040           
  Misses        268      268           
  Partials      218      218           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stevehipwell stevehipwell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gmlewis as an aside did you consider using synctest to keep the timeout pattern?

@gmlewis

gmlewis commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

@gmlewis as an aside did you consider using synctest to keep the timeout pattern?

Yes, in fact the agent started out fixing this flaky test by using synctest but then discovered it could completely remove all asynchrony and make the tests fully deterministic without changing their primary objectives without it.

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jul 10, 2026
@gmlewis gmlewis merged commit e77be1b into google:master Jul 10, 2026
15 checks passed
@gmlewis gmlewis deleted the use-synctest branch July 10, 2026 14:10
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