[trainer] Fix output_remove_retry_attempts not stripping failures for skipped-on-retry tests (modern xcresult parser)#30081
Conversation
… skipped-on-retry tests in modern xcresult parser PR fastlane#29887 taught output_remove_retry_attempts to drop a failure when a test is skipped on retry, but only fixed the legacy xcresult path. In the modern (Xcode 16+) parser, to_xml only trimmed the <testcase> XML nodes to the final attempt while the counts (failures_count / to_hash) were still derived from each test case's aggregate result, which xcresulttool reports as "Failed" even when the final attempt was skipped. Result: the JUnit showed the test as skipped (xcodebuild logged "Test Execute Succeeded") but number_of_failures_excluding_retries stayed at 1, so TestParser#tests_successful? returned false and scan failed CI. Thread output_remove_retry_attempts through the modern parser's count methods so that, only when the flag is set, counts reflect each test case's final attempt (failed?/skipped? based on the last repetition). Behaviour with the flag off is unchanged. Adds a modern-path regression spec mirroring the existing legacy one. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR fixes the modern (Xcode 16+/xcresulttool 23+) XCResult parser so that when output_remove_retry_attempts is enabled, suite/test-plan failure & skipped counts reflect each test case’s final attempt (matching the trimmed JUnit output), preventing CI from reporting failures for fail-then-skip-on-retry tests.
Changes:
- Thread
output_remove_retry_attemptsthrough modern XCResultTestCase/TestSuite/TestPlancounting and hashing logic. - Add
final_result/final_failed?/final_skipped?helpers onTestCaseand use them for retry-stripped counting. - Add a modern-parser regression spec mirroring the legacy regression for fail-then-skip-on-retry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| trainer/spec/test_parser_spec.rb | Adds modern XCResult regression coverage for retry-stripped counting and JUnit attributes. |
| trainer/lib/trainer/xcresult/test_suite.rb | Makes suite counts/hash/XML retry-aware when output_remove_retry_attempts is enabled. |
| trainer/lib/trainer/xcresult/test_plan.rb | Propagates output_remove_retry_attempts into suite hashing and root XML summary counts. |
| trainer/lib/trainer/xcresult/test_case.rb | Introduces final_* helpers and makes total counts optionally reflect only the final attempt. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @foxware00 👋
Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a month, and your PR will be included in the next one.
Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀"
There was a problem hiding this comment.
Congratulations! 🎉 This was released as part of fastlane 2.237.0 🚀
github-actions
Bot
added
status: released
and removed
status: included-in-next-release
labels
Motivation and Context
Follow-up to #29887. That PR taught
output_remove_retry_attemptsto drop a failure when a test is skipped on retry, but only fixed the legacy xcresult path. The modern (Xcode 16+) parser (trainer/lib/trainer/xcresult/) was only half-fixed:TestSuite#to_xmltrimmed the<testcase>XML nodes to the final attempt (runs.last(1)), so the skipped node renders and xcodebuild logs "Test Execute Succeeded."failures_count/to_hash) were still derived from each test case's aggregateresult, whichxcresulttoolreports asFailedeven when the final attempt was skipped — and were never made retry-aware.Result: the JUnit reported the test as
<skipped>while the suite still carriedfailures='1'(with no<failure>element — internally inconsistent), sonumber_of_failures_excluding_retriesstayed at1,TestParser#tests_successful?returnedfalse, and scan reported a failure to CI despite the test run succeeding.Description
Thread
output_remove_retry_attemptsthrough the modern parser's count methods so that — only when the flag is set — counts reflect each test case's final attempt:test_case.rb— addfinal_result/final_failed?/final_skipped?;total_tests_count/total_failures_countaccept the flag.test_suite.rb—failures_count/skipped_count/total_*/to_hash/to_xmlaccept the flag (failures usefinal_failed?, skips usefinal_skipped?when removing retries).test_plan.rb—each(feeds CI pass/fail) and the rootto_xmlsummary pass the flag through.Behaviour with the flag off is unchanged. This matches the legacy regression test's contract under the flag (
failures=0, skipped=1, retries=0).Testing
Trainer::XCResult::TestSuite) mirroring the existing legacy one — fails before, passes after.trainer/spec/test_parser_spec.rb: 21 examples, 0 failures (existing Xcode 26 JUnit fixture tests still pass — no regression; neither checked-in fixture contained a final-result ≠ aggregate case, which is why the bug went uncaught)..xcresultfrom a fail-then-skip-on-retry CI run: with the flag →tests_successful? = true; with the flag off → unchanged (false).Types of changes