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.
| 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.
| $(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.
| 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.
| $(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.
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):