-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[BLD]: fix Go test flakes #4760
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
8f85ef7
to
d4d1398
Compare
be290d9
to
2ef29ad
Compare
2ef29ad
to
6f2e122
Compare
@@ -235,6 +235,10 @@ func GetDBConfigForTesting() DBConfig { | |||
WithStartupTimeout(5*time.Second)), | |||
) | |||
|
|||
if err != nil { | |||
panic(err) |
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.
this is never hit, but realized any error here was swallowed during testing
f0ca173
to
cc91374
Compare
Improve Go Test Reliability by Reducing Flakiness in CI This PR addresses Go test flakes by forcing sequential test execution in the Makefile, disabling the Ryuk container cleanup utility during CI runs, and adding stricter error handling for container startup in test database setup. The changes primarily target conditions causing race conflicts and resource contention with Docker-based database containers in CI environments, aiming to make Go tests more reliable. Key Changes: Affected Areas: This summary was automatically generated by @propel-code-bot |
if err != nil { | ||
panic(err) | ||
} | ||
|
||
var ports nat.PortMap | ||
ports, _ = container.Ports(context.Background()) |
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.
[BestPractice]
There's a missing error check after calling container.Ports()
. Similar to how you've started properly handling the error from postgres2.RunContainer()
, you should also check for errors when getting the ports.
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.
Cool. I guess we might want to keep an eye on this a bit. The tests are already pretty fast and this doesn't seem to slow them down, presumably because there just aren't a ton of them? I guess it would still be nice to know why there are flakes when running in parallel, but since it doesn't "cost" much to run them sequentially, then who cares.
Description of changes
These changes seem to fix most causes of the Go tests flaking. Ran 70 times with no flake, whereas the flake rate seemed to be 5-20% prior to these changes.
Test plan
How are these changes tested?
pytest
for python,yarn test
for js,cargo test
for rust