fix: avoid sharing echo.Responses across tests
#17211
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
echo.Responses across tests
There was a problem hiding this comment.
Pull Request Overview
This PR refactors test initialization for echo.Responses to eliminate shared mutable state between tests, addressing flakiness related to the echo provisioner.
- Convert echo.Responses variables into factory functions that return new instances.
- Update test cases to invoke these factories to avoid data sharing across test runs.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cli/update_test.go | Wrap echo.Responses initialization in a function and update its usages. |
| cli/start_test.go | Replace direct echo.Responses assignment with a factory function invocation. |
| cli/restart_test.go | Apply factory function pattern to create new echo.Responses instances. |
Comments suppressed due to low confidence (4)
cli/update_test.go:104
- [nitpick] Consider renaming 'echoResponses' to 'newEchoResponses' to clarify that it returns a fresh instance on each invocation.
echoResponses := func() *echo.Responses {
cli/start_test.go:82
- [nitpick] Consider renaming 'echoResponses' to 'newEchoResponses' for clarity since it now functions as a factory method.
echoResponses := func() *echo.Responses {
cli/restart_test.go:23
- [nitpick] Consider renaming 'echoResponses' to 'newEchoResponses' to better indicate that it creates a new instance each time it is called.
echoResponses := func() *echo.Responses {
cli/restart_test.go:285
- [nitpick] Consider renaming 'echoResponses' to 'newEchoResponses' to clarify its role as a factory function for generating new instances.
echoResponses := func() *echo.Responses {
|
sreya
approved these changes
Apr 2, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes coder/internal#551
We've noticed lots of flakes in
go test -racetests that use the echo provisioner. I believe the root cause of this to be #17012, where we started mutating theecho.Responses. This only caused issues as we previously sharedecho.Responsesacross multiple test cases.This PR is therefore the same as #17128, but I believe this is all the cases where an
echo.Responsesis shared between tests - including tests that haven't flaked (yet).