Skip to content

test: deterministic debounce test via synctest#745

Merged
buchdag merged 5 commits into
nginx-proxy:mainfrom
JamBalaya56562:fix/238-deterministic-debounce-test
Jul 10, 2026
Merged

test: deterministic debounce test via synctest#745
buchdag merged 5 commits into
nginx-proxy:mainfrom
JamBalaya56562:fix/238-deterministic-debounce-test

Conversation

@JamBalaya56562

Copy link
Copy Markdown
Contributor

Summary

TestGenerateFromEvents drives the debounce logic through a real dockertest HTTP server and wall-clock time.Sleep calls, so the timing of debounce firings races the OS scheduler / timer resolution and the test fails nondeterministically (off-by-one or worse). It reproduces on main and is especially flaky on Windows. See #238.

This PR replaces it with TestNewDebounceChannel, which exercises newDebounceChannel directly inside a testing/synctest bubble. The fake clock makes the Min quiet-period coalescing and the Max cap fire at exact, reproducible virtual times, removing the real network and wall-clock dependence entirely.

Changes

  • internal/generator/generator_test.go: replace TestGenerateFromEvents with TestNewDebounceChannel. Three deterministic subtests:
    • passthrough when Min == 0,
    • Min quiet-period coalescing (a burst collapses to one event, fired Min after the last event),
    • Max cap (output is forced out at Max while events keep arriving).

Notes

  • No production code changes. newDebounceChannel already uses time.After, which synctest virtualizes.
  • testing/synctest is stable as of Go 1.25 (go.mod declares go 1.25.5; the CI matrix tests Go 1.25 and 1.26). It is OS-independent, which removes the Windows timer-resolution flakiness at its root.
  • Verified deterministic: go test -run TestNewDebounceChannel -count=200 passes.
  • Trade-off: this focuses on the debounce timing (the logic that was actually flaky) rather than the full event → generate → output pipeline that the old test wrapped around a real Docker API server. If you'd prefer to keep an integration smoke test for that wiring (without timing assertions, so it stays deterministic), I'm happy to add one.

Fixes #238

@JamBalaya56562 JamBalaya56562 force-pushed the fix/238-deterministic-debounce-test branch from 5ea2754 to b916e5c Compare June 18, 2026 14:50
@buchdag buchdag requested a review from Copilot June 19, 2026 12:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR removes the flaky, wall-clock/network-driven debounce integration test (TestGenerateFromEvents) and replaces it with deterministic unit tests for newDebounceChannel using Go’s testing/synctest, addressing nondeterministic timing failures reported in #238.

Changes:

  • Replaced TestGenerateFromEvents (dockertest server + real time.Sleep) with TestNewDebounceChannel built on testing/synctest.
  • Added deterministic subtests covering: Min == 0 passthrough, Min quiet-period coalescing, and Max cap behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/generator/generator_test.go
@JamBalaya56562 JamBalaya56562 force-pushed the fix/238-deterministic-debounce-test branch 2 times, most recently from 9632402 to 6cbdd3a Compare June 20, 2026 11:55
@JamBalaya56562 JamBalaya56562 changed the title test: make the debounce test deterministic with testing/synctest (#238) test: deterministic debounce test via synctest (#238) Jun 20, 2026
@buchdag buchdag changed the title test: deterministic debounce test via synctest (#238) test: deterministic debounce test via synctest Jun 20, 2026
@buchdag

buchdag commented Jun 20, 2026

Copy link
Copy Markdown
Member

@JamBalaya56562 same thing here, no issue or PR numbers in either PR titles or commit message.

@JamBalaya56562 JamBalaya56562 force-pushed the fix/238-deterministic-debounce-test branch from 6cbdd3a to 6b64045 Compare June 20, 2026 12:01
@JamBalaya56562

Copy link
Copy Markdown
Contributor Author

Done — dropped the issue number from the title and commit message, thanks!

@JamBalaya56562 JamBalaya56562 force-pushed the fix/238-deterministic-debounce-test branch from 6b64045 to 56ac89c Compare June 20, 2026 14:04
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@JamBalaya56562 JamBalaya56562 force-pushed the fix/238-deterministic-debounce-test branch from 56ac89c to 5e508d0 Compare June 28, 2026 12:32
Comment thread internal/generator/generator_test.go Outdated
JamBalaya56562 and others added 3 commits July 10, 2026 07:05
Address review feedback on PR nginx-proxy#745: remove the verbose doc comment
and overly explanatory inline comments, keeping only concise timing
markers.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The merge with main pulled in TestGetContainersNilStructs, which uses
encoding/json, fmt, net/http, strings, dockertest and dockerclient.
These imports had been removed together with TestGenerateFromEvents,
breaking the build after the merge. Restore them.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@JamBalaya56562

Copy link
Copy Markdown
Contributor Author

Separately: now that #748 (TestGetContainersNilStructs) is merged into main, updating this branch pulled that test in. I've added a commit restoring the imports it needs (encoding/json, fmt, net/http, strings, dockertest, dockerclient), which had been dropped along with TestGenerateFromEvents. Build/tests pass locally now.

@JamBalaya56562 JamBalaya56562 requested a review from buchdag July 10, 2026 07:24
@buchdag buchdag merged commit 1f702ff into nginx-proxy:main Jul 10, 2026
6 checks passed
@JamBalaya56562 JamBalaya56562 deleted the fix/238-deterministic-debounce-test branch July 10, 2026 09:34
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.

TestGenerateFromEvents race condition

3 participants