-
Notifications
You must be signed in to change notification settings - Fork 2
ci: rerun flaky tests #256
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
Simplifies our Makefile by combining some targets and adds an option to rerun flaky tests. This option is useful because we have some occasional port conflict issues when the OS assigns an ephemeral port that we're going to use for an instance of Etcd or other service. These issues are extremely difficult to avoid because we can't easily eliminate the gap between our port allocation and when the service binds to the ports. Simply re-running the tests is a much easier option.
📝 WalkthroughWalkthroughThe CI and Makefile gains test rerun support (TEST_RERUN_FAILS), linting is unified, health assertions now include JSON-formatted component diagnostics, and container log handling is refactored into a reusable test helper. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@clustertest/host_test.go`:
- Around line 121-124: The cleanup path uses t.Context() (which is canceled)
when calling printContainerLogs, causing log extraction to fail; update
printContainerLogs calls to use the active cleanup context instead of
t.Context() — either change the helper signature to accept a ctx (e.g.,
printContainerLogs(ctx, t, id, ctr)) and pass the non-canceled cleanup ctx, or
have printContainerLogs create a short-lived background/timeout context
internally (context.WithTimeout(context.Background(), ...)) before fetching
logs; apply the same fix to the other printContainerLogs usages in this test.
d333f55 to
146650c
Compare
rshoemaker
left a comment
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.
approved.
Summary
Simplifies our Makefile by combining some targets and adds an option to rerun flaky tests. This option is useful because we have some occasional port conflict issues when the OS assigns an ephemeral port that we're going to use for an instance of Etcd or other service. These issues are extremely difficult to avoid because we can't easily eliminate the gap between our port allocation and when the service binds to the ports. Simply re-running the tests is a much easier option.
Testing
See the
.circleci.yamlfile for the changed commands.Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.