Stop launching the app in sample UI tests that never use it - #423
Merged
Merged
Conversation
Closes #398. FirstSuite, SecondSuite and ThirdSuite each called XCUIApplication().launch() in setUp with continueAfterFailure = false. When the simulator was slow, the launch failed and took the test with it before its body ran — FirstSuite.testOne is XCTAssert(true) plus an attachment and cannot fail on its own assertion, yet it was observed failing. No test in the sample app touches the app's UI. There are no taps, no element queries, no XCUIApplication use beyond the launch itself. The launch existed only to make these genuine UI tests, on the assumption that the screen recording depended on it. It does not. Verified by controlled experiment: removed the launch from SecondSuite alone and regenerated — the fixture still contained 9 distinct screen recordings, exactly as before. Had SecondSuite lost its recordings the count would have dropped by two. Xcode captures the simulator screen for UI test targets regardless of whether the app runs. So the launches were pure flake risk with no fixture value. Fixture generation also drops from 85.7s to 50.4s locally, a 41% saving, because three suites no longer launch and terminate an app per test method. Whether that carries to CI is for CI to measure — see #412 for why local timing is not evidence here. One honest cost: SanityResults.xcresult shrinks from ~51M to ~140K, because a recording of an idle screen is much smaller than one of a launching app. TestResults still carries 9 recordings and 16 .mp4 references, so .mp4 attachment coverage — the reason the launches were thought necessary — is intact. Verified: swift test passes 23 tests with 1 skipped; TestResults reports All(16) Passed(9) Skipped(1) Failed(6), consistent with the suite after the Swift Testing fixture in #417.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughThe three sample UI test suites no longer launch ChangesUI test setup
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 11, 2026
tylervick
added a commit
that referenced
this pull request
Aug 11, 2026
Generating a report from TestResults.xcresult on main produced 16 .mp4 references, 0 .png/.jpeg, and 0 rendered `img class="screenshot"` elements. The screenshot templates, the `screenshot` CSS class and the `-z` downsizing branch were executed by no test at all -- that path could have been entirely broken and `swift test` would still have been green. #357 (screenshots missing from reports, open since June 2024) has been uninvestigable for the same reason. Adds FirstSuite.testAttachScreenshot, which attaches XCUIScreen.main.screenshot() with .keepAlways. That yields a real public.png payload, which Attachment.swift maps to isImage -> cssClass "screenshot". It deliberately does NOT launch the app: XCUIScreen captures the simulator screen without it, and #423 removed the launches from these suites as pure flake risk. Restores the image assertion in CliTests, commented out when Xcode 15 began attaching videos by default. Widened to include `img.screenshot-tail`, which the original selector missed, and given failure messages that say what is wrong. Proved non-vacuous rather than assumed: removing the screenshot test and regenerating makes it fail on BOTH the plain and the -z variants ("No image attachments rendered"). Restoring it returns the suite to green. Also: - CoreTests expected 16 rows; there are now 17 (14 XCTest + 3 Swift Testing). Its comment justified not asserting the pass/fail split on the grounds that the suites launch the app in setUp -- false since #423. Rewritten to say what is actually true. - Excludes XCTestHTMLReportSampleApp/.derivedData from SwiftLint. #425 moved DerivedData inside the repo; SwiftLint does not read .gitignore and reported errors from Xcode-generated sources. CI never saw it because the lint job does not generate fixtures, so it only bit locally. swift test: 23 tests, 1 skipped, 0 failures. Closes #393. Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
tylervick
added a commit
that referenced
this pull request
Aug 12, 2026
Fixture generation boots a simulator and runs the sample app's UI tests on every CI run; even after #425 it dominates the test job's ~10-minute wall time (#412). The bundles are deterministic for a given toolchain and sample app (#423/#430), and every assertion in the suite compares within one run, so identical bytes are safe to serve across runs. Cache the three .xcresult bundles keyed on the exact Xcode build, the newest installed iOS simulator runtime, prepareTestResults.sh itself, and the sample-app sources. No restore-keys: an inexact match would serve fixtures from a different toolchain, which the drift detector (#392) and the legacy-vs-modern differential test cannot tolerate. A verification step asserts all three bundles exist whether restored or generated, so a cache hit that restores nothing fails instead of passing as a suspiciously fast green run. Local flow is untouched: prepareTestResults.sh is unchanged and swift test still reads the same paths. Fixes #412 Co-authored-by: Claude Fable 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #398.
The flake
FirstSuite,SecondSuiteandThirdSuiteeach calledXCUIApplication().launch()insetUpwithcontinueAfterFailure = false. When the simulator was slow the launch failed and took the test with it —FirstSuite.testOneisXCTAssert(true)plus an attachment and cannot fail on its own assertion, yet was observed failing (53adfafpassed underTestand failed underCodecov, same commit, same commands).No test uses the UI
There are no taps, no element queries, and no
XCUIApplicationuse anywhere in the sample app beyond the launch itself. The launch existed to make these genuine UI tests, on the assumption that the screen recording depended on it.That assumption was wrong, and I tested it rather than reasoning about it
Removed the launch from
SecondSuitealone, regenerated, and counted recordings in the fixture:Had
SecondSuitelost its recordings the count would have dropped by two. Xcode captures the simulator screen for UI test targets regardless of whether the app runs.So the launches were pure flake risk with no fixture value.
Side effect: fixture generation is 41% faster locally
85.7s → 50.4s, because three suites no longer launch and terminate an app per test method.Whether that carries to CI is for CI to measure — #412 records why local timing is not evidence for this repository, and the last attempt at that issue (#422) was closed after CI showed it slower despite a plausible local story. Same discipline applies here.
One honest cost
SanityResults.xcresultshrinks from ~51M to ~140K, because a recording of an idle screen is much smaller than one of a launching app.TestResults.xcresultstill carries 9 recordings and 16.mp4references, so.mp4attachment coverage — the reason the launches were believed necessary — is intact. If a fixture is later needed that genuinely exercises a launching app, it should be one suite that does so deliberately, not every suite by default.Verified
swift test→ 23 tests, 1 skipped, 0 failuresTestResultsreportsAll(16) Passed(9) Skipped(1) Failed(6), consistent with the suite after Add Swift Testing (@Test) fixture and structural assertions #417's Swift Testing fixtureunneeded_overrideSwiftLint warnings are pre-existing onmain(emptytearDownoverrides) and untouched here🤖 Generated with Claude Code
Summary by CodeRabbit