test(blocker): add synctest to blocker pkg #5258
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors blocker tests to use Go's testing/synctest package for more reliable time-based testing. The changes wrap existing test logic in synctest.Test() calls, which provides deterministic control over time during testing.
- Wraps three test functions with
synctest.Test()for deterministic time control - Temporarily restricts CI test scope to
pkg/blockerto speed up testing during development
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pkg/blocker/blocker_test.go | Wrapped TestBlocksAfterFlagTimeout, TestUnflagBeforeBlock, and TestPruneBeforeBlock with synctest.Test() for deterministic time-based testing |
| Makefile | Temporarily narrowed test scope from ./... to ./pkg/blocker in CI test targets with TODO comment indicating this should be reverted before merge |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Makefile
Outdated
| # TODO: should be ./... before merging, temporary change to pkg/blocker to speed up CI while testing blocker changes | ||
| $(GO) test -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/blocker |
There was a problem hiding this comment.
The test scope has been temporarily narrowed to ./pkg/blocker only. As noted in the TODO comment, this should be reverted to ./... before merging to ensure full test coverage across the codebase.
Makefile
Outdated
| $(GO) test -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/blocker | ||
| else | ||
| $(GO) test -run "[^FLAKY]$$" ./... | ||
| $(GO) test -run "[^FLAKY]$$" ./pkg/blocker |
There was a problem hiding this comment.
The test scope has been temporarily narrowed to ./pkg/blocker only. This should be reverted to ./... before merging to ensure full test coverage across the codebase.
Makefile
Outdated
| test-ci-race: | ||
| ifdef cover | ||
| $(GO) test -race -run "[^FLAKY]$$" -coverprofile=cover.out ./... | ||
| $(GO) test -race -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/blocker |
There was a problem hiding this comment.
The test scope has been temporarily narrowed to ./pkg/blocker only. This should be reverted to ./... before merging to ensure full test coverage across the codebase.
Makefile
Outdated
| $(GO) test -race -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/blocker | ||
| else | ||
| $(GO) test -race -run "[^FLAKY]$$" ./... | ||
| $(GO) test -race -run "[^FLAKY]$$" ./pkg/blocker |
There was a problem hiding this comment.
The test scope has been temporarily narrowed to ./pkg/blocker only. This should be reverted to ./... before merging to ensure full test coverage across the codebase.
pkg/blocker/blocker_test.go
Outdated
| if len(blockedC) != 0 { | ||
| t.Fatal("blocker did not wait flag duration") | ||
| } | ||
| time.Sleep(flagTime / 2) |
There was a problem hiding this comment.
it appears that the idiomatic way to use the synctest package is to use the synctest.Wait() call inside the tests rather than reverting to the inlined time.Sleep. WDYT about changing those sleep calls to the wait call?
There was a problem hiding this comment.
Yes, it makes sense.
I will change them.
Thanks for the review 🙂
pkg/blocker/blocker_test.go
Outdated
| time.Sleep(flagTime * 3) | ||
| // Suspending current goroutine expect that in this interval | ||
| // block listener was not called to block flagged address | ||
| time.Sleep(flagTime * 3) |
There was a problem hiding this comment.
not sure if this is really needed. if it is, then it could probably also be replaced with synctest.Wait
There was a problem hiding this comment.
Yes, i forget it.
I updated it, thanks
Checklist
Description
Add
synctestpackage inblockertests to prevent flakiness.Remove
parallelexecution from tests.Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
#5232
Screenshots (if appropriate):