test(pullsync): add synctest to pullsync #5261
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates pullsync tests to use Go 1.25's new testing/synctest package, replacing t.Parallel() calls with synctest.Test() wrappers. This allows for deterministic testing of concurrent code by controlling goroutine scheduling. The Makefile is also temporarily modified to run only pullsync tests during CI.
- Wrapped 9 test functions with
synctest.Test()to enable deterministic concurrent testing - Removed
t.Parallel()calls that are incompatible with synctest - Temporarily restricted CI test scope to
./pkg/pullsyncfor validation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/pullsync/pullsync_test.go | Wrapped test functions with synctest.Test() and removed t.Parallel() calls to enable deterministic concurrent testing |
| Makefile | Temporarily restricted test-ci and test-ci-race targets to only run pullsync tests instead of all tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Makefile
Outdated
| $(GO) test -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/pullsync | ||
| else | ||
| $(GO) test -run "[^FLAKY]$$" ./... | ||
| $(GO) test -run "[^FLAKY]$$" ./pkg/pullsync |
There was a problem hiding this comment.
The Makefile changes restrict CI testing to only ./pkg/pullsync instead of ./.... This appears to be a temporary change for testing purposes but should not be merged to production. If this is intentional for this PR, it should be reverted in a follow-up commit or before merging to ensure all package tests continue to run in CI.
Makefile
Outdated
| $(GO) test -race -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/pullsync | ||
| else | ||
| $(GO) test -race -run "[^FLAKY]$$" ./... | ||
| $(GO) test -race -run "[^FLAKY]$$" ./pkg/pullsync |
There was a problem hiding this comment.
The Makefile changes restrict CI testing to only ./pkg/pullsync instead of ./.... This appears to be a temporary change for testing purposes but should not be merged to production. If this is intentional for this PR, it should be reverted in a follow-up commit or before merging to ensure all package tests continue to run in CI.
akrem-chabchoub
requested review from
acud,
gacevicljubisa,
janos and
martinconic
Checklist
Description
Add synctest in pullsync.
Remove parallel execution from tests (synctest).
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
#5232
Screenshots (if appropriate):