CI: make the configured test retries actually absorb a flake - #2274
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe Fastlane test configuration now removes superseded retry attempts from ChangesTest retry result handling
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change is narrowly scoped to ensuring retried tests are evaluated by their final result, with no unresolved merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. A rabbit checks the test array, Comment |
number_of_retries made xcodebuild re-run failed tests, but scan judged the lane on every attempt, so a test that failed once and passed on retry still failed CI. The retry was therefore doing nothing it was configured for, while the comment beside it claimed otherwise. Seen on the UTS iOS leg: three suites failed, retried, and passed; xcodebuild reported Test run with 752 tests passed after 578.122 seconds Test Succeeded and scan then reported "Number of failures | 3" and failed the lane. output_remove_retry_attempts makes scan judge each test on its final state. A genuine regression still fails, because it fails every attempt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
output_remove_retry_attempts does not strip a failure when the retry skipped the test rather than passing it, so a flake still failed CI even with retries configured: xcodebuild reported "Test Succeeded" while scan reported one failure. 2.237.0 carries the fix for that (#30081). 2.237.0 rather than the current 2.239.0: it is the earliest release with the fix, and it leaves faraday on 1.10.6, where 2.239.0 moves it to 2.x. Gemfile is unchanged; the dependency stays unpinned and the lockfile records the resolved version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d295d15 to
62ef761
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Split out of #2268. Stacked on #2273 (base:
fix/uts-issue-recording), which is what makes flakes retryable in the first place.Preparation for #2275
Why
The Fastlane lanes have set
number_of_retries: 2for a long time, and the comment beside it claims the retry "absorbs single-occurrence environmental timeouts on the CI runners". That was not true. Two separate reasons, one per commit.1. scan judged the lane on every attempt.
output_remove_retry_attemptsdefaults tofalse, so a test that failed once and passed on retry still failed CI. Observed on a UTS leg: three suites failed, retried, and passed —xcodebuild said
Test Succeeded; scan failed the lane on the superseded attempts. So the retry was doing nothing it was configured for.2. fastlane 2.227.2 does not strip a failure when the retry skipped the test rather than passing it, so one phantom failure survived even with the option on (3 → 1). That is fastlane #30081, fixed in 2.237.0.
What
output_remove_retry_attempts: truein the shared lane arguments, with the comment corrected.Gemfile.lockmoved to fastlane 2.237.0 — the earliest release carrying the fix. Deliberately not 2.239.0, which also moves faraday to 2.x; 2.237.0 leaves it on 1.10.6.Gemfileis unchanged, so the dependency stays unpinned.This does not weaken the signal: a genuine regression fails every attempt and still fails the lane. The option only discards attempts a later retry superseded.
It applies to every lane, so
Integration Testbenefits as much as the UTS legs — which is the point, since the flaw was never UTS-specific.Verification
The argument mapping was checked by evaluating
LaneConfigdirectly (number_of_retries=2 output_remove_retry_attempts=true), plusruby -con the Fastfile.bundle lockre-resolves to the same lockfile against the unpinnedGemfile, so the pin is not accidental.One gap worth flagging:
bundle installfails on my machine for unrelated reasons —bigdecimal,jsonandnkfcannot build native extensions into a.sitearchdir— so I have no local proof the bundle installs. CI'sInstall Dependenciesstep on a clean runner is the first real test of the lockfile, and it fails early and unmistakably if the resolution is bad.🤖 Generated with Claude Code
Summary by CodeRabbit