- Notifications
You must be signed in to change notification settings - Fork911
fix: avoid sharingecho.Responses across tests#17211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
echo.Responses across testsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 {ethanndickson commentedApr 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Many blessings upon your house 🙏
6fdad02 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closescoder/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).