Stop asserting simulator reliability in testResultStatusCount - #397
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The Codecov run on main failed while the Test run on the same commit passed: 53adfaf produced Passed(6)/Failed(6) instead of Passed(7)/Failed(5). FirstSuite.testOne appears as both passed and failed across the fixture generation logs. Its body is XCTAssert(true) plus an attachment, so it cannot fail on its own assertion — the failure comes from setUp, which calls XCUIApplication().launch() with continueAfterFailure = false. A slow simulator turns a would-be pass into a failure. This became reachable when fixtures started being regenerated on every run instead of downloaded as fixed artifacts. Exact per-bucket counts were safe against byte-identical fixtures; they are not against freshly generated ones. Assert what the sample sources actually determine — total, skipped, mixed, the bucket-sum invariant, and a floor on deliberate failures — and stop asserting the pass/fail split, which measures the simulator rather than this project. A lost test still breaks the "all == 13" assertion, dropped failures still break "failed >= 5", and miscounting still breaks the sum. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9d9f916 to
895611c
Compare
1 participant
The
Codecovrun onmainis failing.Teston the same commit passes.Both run
./prepareTestResults.sh && swift test. Same code, same commit, different outcome — so this is nondeterminism, not a code defect.What happens
Passedwent 7→6 andFailedwent 5→6; the total stayed 13. One test moved buckets. The fixture-generation log showsFirstSuite.testOneas both passed and failed:Its body is
XCTAssert(true)plus an attachment — it cannot fail on its own assertion. The failure comes fromsetUp, which callsXCUIApplication().launch()withcontinueAfterFailure = false. Under CI load the simulator is sometimes slow to launch the app, and the test fails before its body runs.Why it started now
This is a consequence of #379. Fixtures used to be fixed artifacts downloaded from a private bucket — byte-identical every run, so exact per-bucket assertions were safe. They are now regenerated on every run, which is a better trade overall (they can't go stale, and fork PRs can run them), but it means UI-test flakiness feeds straight into assertions that were written for stable input.
The fix
Assert only what the sample sources actually determine:
all == 13skipped == 1SampleAppUnitTests.testSkippedis an unconditionalXCTSkipIfmixed == 0TestResultsexcludesRetryTestspassed + failed == all - skippedfailed >= 5The exact pass/fail split is dropped, because it measures whether the simulator launched an app in time — not anything this project does.
This still catches real regressions. Losing a test breaks
all == 13. Silently dropping failures breaksfailed >= 5. Miscounting breaks the sum invariant. Under the flaked run (passed=6, failed=6) the new assertions correctly pass.testMixedStatusis left alone: it readsRetryResults, andRetryTestshas nosetUpand never launches the app, so it isn't exposed to this.Follow-up
The underlying flakiness is worth its own issue — the sample UI tests launch the app purely so the run produces screen recordings (the only
.mp4fixture coverage), and none of them actually interact with the UI. There may be a way to keep the recordings without the launch being a failure point.🤖 Generated with Claude Code