test(libp2p): update reacher tests to use synctest #5267
Conversation
akrem-chabchoub
requested review from
Copilot,
gacevicljubisa,
janos and
martinconic
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes test timing control by adopting Go's testing/synctest package for time-sensitive tests in the reacher component. The main goal is to replace traditional time-based testing with deterministic, controllable time manipulation.
- Replaced
t.Parallel()withsynctest.Test()in two test functions - Added
testing/synctestimport - Modified Makefile to scope CI test runs to the reacher package
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/p2p/libp2p/internal/reacher/reacher_test.go | Wrapped test bodies with synctest.Test() and removed t.Parallel() calls to enable deterministic time control in timing-sensitive tests |
| Makefile | Narrowed test-ci and test-ci-race targets to run only the reacher package tests instead of all tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test-ci: | ||
| ifdef cover | ||
| $(GO) test -run "[^FLAKY]$$" -coverprofile=cover.out ./... | ||
| $(GO) test -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/p2p/libp2p/internal/reacher |
There was a problem hiding this comment.
The test-ci target has been narrowed to run only the reacher package tests. This breaks the CI testing for the rest of the codebase. If this is intended for testing purposes only, it should not be committed. Otherwise, revert to ./... to ensure comprehensive test coverage in CI.
| $(GO) test -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/p2p/libp2p/internal/reacher | ||
| else | ||
| $(GO) test -run "[^FLAKY]$$" ./... | ||
| $(GO) test -run "[^FLAKY]$$" ./pkg/p2p/libp2p/internal/reacher |
There was a problem hiding this comment.
The test-ci target has been narrowed to run only the reacher package tests. This breaks the CI testing for the rest of the codebase. If this is intended for testing purposes only, it should not be committed. Otherwise, revert to ./... to ensure comprehensive test coverage in CI.
| test-ci-race: | ||
| ifdef cover | ||
| $(GO) test -race -run "[^FLAKY]$$" -coverprofile=cover.out ./... | ||
| $(GO) test -race -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/p2p/libp2p/internal/reacher |
There was a problem hiding this comment.
The test-ci-race target has been narrowed to run only the reacher package tests. This breaks the CI race detection testing for the rest of the codebase. If this is intended for testing purposes only, it should not be committed. Otherwise, revert to ./... to ensure comprehensive test coverage in CI.
| $(GO) test -race -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/p2p/libp2p/internal/reacher | ||
| else | ||
| $(GO) test -race -run "[^FLAKY]$$" ./... | ||
| $(GO) test -race -run "[^FLAKY]$$" ./pkg/p2p/libp2p/internal/reacher |
There was a problem hiding this comment.
The test-ci-race target has been narrowed to run only the reacher package tests. This breaks the CI race detection testing for the rest of the codebase. If this is intended for testing purposes only, it should not be committed. Otherwise, revert to ./... to ensure comprehensive test coverage in CI.
Checklist
Description
Add synctest in reacher test.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
#5232
Screenshots (if appropriate):