Conversation
akrem-chabchoub
changed the title
…nt results for flaky tests
akrem-chabchoub
force-pushed
the
migrate-to-synctest
branch
from
49cbac0 to
88fae9b
Compare
akrem-chabchoub
requested review from
Copilot,
gacevicljubisa,
janos and
martinconic
There was a problem hiding this comment.
Pull Request Overview
This PR integrates the testing/synctest package to modernize time-dependent tests across the codebase. The primary goal is to replace real time delays with virtual time control, making tests more deterministic and faster.
Key changes:
- Wraps existing test functions with
synctest.Test()to enable virtual time - Updates context creation from
context.Background()tot.Context()for proper integration - Adjusts timing assertions to account for virtual time behavior (e.g., RTT can be 0)
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/syncutil/syncutil_test.go | Wrapped timeout test with synctest |
| pkg/storer/internal/reserve/reserve_test.go | Added synctest to eviction test |
| pkg/storer/internal/events/subscribe_test.go | Integrated synctest for event subscription test |
| pkg/storageincentives/soc_mine_test.go | Added synctest wrapper to mining test |
| pkg/storageincentives/agent_test.go | Wrapped agent tests with synctest and added goroutine wait |
| pkg/spinlock/wait_test.go | Integrated synctest for spinlock timeout tests |
| pkg/pullsync/pullsync_test.go | Added synctest to multiple pull sync protocol tests |
| pkg/pingpong/pingpong_test.go | Updated RTT validation logic and removed Windows-specific timing workaround |
| pkg/file/joiner/joiner_test.go | Migrated context creation and refactored redundancy tests with synctest |
| pkg/blocker/blocker_test.go | Wrapped blocker timeout tests with synctest |
| pkg/api/gsoc_test.go | Added blank line for formatting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| return | ||
| } | ||
| } | ||
|
|
||
| assertOrder := func(t *testing.T, want, got contractCall) { | ||
| t.Helper() | ||
| if want != got { | ||
| t.Fatalf("expected call %s, got %s", want, got) | ||
| if len(contract.callsList) == 0 { | ||
| t.Fatal("expected calls but got none") |
There was a problem hiding this comment.
The logic flow is inverted: the code checks for no calls when expectedCalls is false, then returns, but then checks for no calls again when expectedCalls is true. This creates duplicate validation. Consider restructuring: move the zero-length check (line 136-138) before the !tc.expectedCalls block to avoid redundancy.
|
Will split it to multiple PRs |
Checklist
Description
testing/synctestfor deterministic, faster, and less flaky tests.Files migrated to synctest
pkg/blocker/blocker_test.gopkg/file/joiner/joiner_test.gopkg/pingpong/pingpong_test.gopkg/pullsync/pullsync_test.gopkg/spinlock/wait_test.gopkg/storageincentives/agent_test.gopkg/storageincentives/soc_mine_test.gopkg/storer/internal/events/subscribe_test.gopkg/storer/internal/reserve/reserve_test.gopkg/util/syncutil/syncutil_test.goOpen API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
#5232
Screenshots (if appropriate):