-
Notifications
You must be signed in to change notification settings - Fork 499
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
(Should fix) #3789: Fix flaky test retry mechanism for Bazel CI #3908
Conversation
Reopening to trigger run-all-tests workflow. |
Follow-up: I can't seem to unlink #3789 so I'll need to manually reopen it. |
TODO script failure is probably due to an out-of-date base branch. Syncing to see if CI now passes. Edit: it seems to be passing now. |
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.
As this can be verified after few days of merging so this LGTM and we can merge it.
Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
Thanks @anandwana001. Merging now. |
Explanation
(Should fix) #3789
See #3789 (comment) for context on the root issue. This PR fixes the retry mechanism by temporarily disabling early exit when performing the Bazel buld retry, and then re-enables it & conditionally exits with error code if the last command fails (since Bazel should guarantee that if any of the iterations result in a passing build, all subsequent ones should).
Note that actually fixing this does mean that legitimate build failures will result in spammier output since they'll be retried multiple times, but that should probably be fine until we can fix the Jettifier issue long-term (#3759).
Finally, I can't actually seem to verify that this is working until the PR is merged (I tried multiple times in #3790 to trigger a scenario in the PR, but couldn't). I think it's reasonable to merge this as-is, and then close #3789 once it's been verified (I'm intentionally not linking the PR & issue for this reason since we can't be sure it's fixed until a few days after merging).
For partial verification:
Essential Checklist
For UI-specific PRs only
N/A