test(storer-reserve): add synctest in evict test #5263
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the TestEvict function to use synctest.Test instead of running in parallel, likely to enable deterministic testing of time-dependent behavior. The Makefile is temporarily modified to run only the reserve package tests.
- Replaced
t.Parallel()withsynctest.Test()wrapper inTestEvict - Wrapped the entire test body in a function closure passed to
synctest.Test - Temporarily narrowed test-ci targets to only test the reserve package
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/storer/internal/reserve/reserve_test.go | Refactored TestEvict to use synctest.Test for controlled time execution |
| Makefile | Temporarily restricted test-ci and test-ci-race targets to only run reserve package tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Makefile
Outdated
| test-ci: | ||
| ifdef cover | ||
| $(GO) test -run "[^FLAKY]$$" -coverprofile=cover.out ./... | ||
| $(GO) test -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/storer/internal/reserve/ |
There was a problem hiding this comment.
The test-ci targets are temporarily restricted to only run ./pkg/storer/internal/reserve/ tests instead of the full test suite (./...). This appears to be a temporary debugging/development change that should not be committed. These targets should test the entire codebase in CI.
Makefile
Outdated
| $(GO) test -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/storer/internal/reserve/ | ||
| else | ||
| $(GO) test -run "[^FLAKY]$$" ./... | ||
| $(GO) test -run "[^FLAKY]$$" ./pkg/storer/internal/reserve/ |
There was a problem hiding this comment.
The test-ci targets are temporarily restricted to only run ./pkg/storer/internal/reserve/ tests instead of the full test suite (./...). This appears to be a temporary debugging/development change that should not be committed. These targets should test the entire codebase in CI.
Makefile
Outdated
| test-ci-race: | ||
| ifdef cover | ||
| $(GO) test -race -run "[^FLAKY]$$" -coverprofile=cover.out ./... | ||
| $(GO) test -race -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/storer/internal/reserve/ |
There was a problem hiding this comment.
The test-ci-race targets are temporarily restricted to only run ./pkg/storer/internal/reserve/ tests instead of the full test suite (./...). This appears to be a temporary debugging/development change that should not be committed. These targets should test the entire codebase in CI.
Makefile
Outdated
| $(GO) test -race -run "[^FLAKY]$$" -coverprofile=cover.out ./pkg/storer/internal/reserve/ | ||
| else | ||
| $(GO) test -race -run "[^FLAKY]$$" ./... | ||
| $(GO) test -race -run "[^FLAKY]$$" ./pkg/storer/internal/reserve/ |
There was a problem hiding this comment.
The test-ci-race targets are temporarily restricted to only run ./pkg/storer/internal/reserve/ tests instead of the full test suite (./...). This appears to be a temporary debugging/development change that should not be committed. These targets should test the entire codebase in CI.
acud
left a comment
acud
left a comment
There was a problem hiding this comment.
Some of the actual test code has been reshuffled/changed. In general, even if it is understood, it is better to have as a separate change. The diff here is bigger than expected. If we're just adding synctest is there any reason why the diff should be more than just the synctest.Test wrapper?
If you're using VSCode, these extensions can help avoid noisy diffs and make reviews much clearer: GitHub Pull Requests: SemanticDiff: 
|
|
Thanks @akrem-chabchoub. I don't use VSCode :) Hopefully the github diff viewer recovers from its sicknesses soon. |
Checklist
Description
Add synctest in evict test.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
#5232
Screenshots (if appropriate):