perf(ios): shorten smoke critical path with affected XCTest selection - #2896
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at f9ce9ab. Running the runner XCTests only when their inputs change should cut iOS smoke time on PRs that do not touch the runner, and unknown diffs fail open. The new Not blocking: I read the scripts and workflow diffs but did not run the new tests. Both Smoke Tests jobs were still running at review time, and this PR edits the workflows they run, so a failure there would likely be related. The next step is your answer on the shared ownership predicate. |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Thermo-nuclear structural pass (on the CI/skip logic and the selection model, not "does the job run"). What is right: RUNNER_INPUTS is a clean declarative prefix/exact input set (no if path matches ... chain), it genuinely fails open on non-PR / null / empty change sets, and choosing to test cache-inputs ⊆ selection-inputs rather than a test-name list is the correct invariant to reach for; the reorder that puts the cheap gesture pan canary before the long fixture E2E is sound.
Three structural concerns, all one theme — put the input sets where the repo already enforces input sets, and make the boundary fail the same way the module does. The replay edit (removing three unasserted captures, reorder) I checked and have no finding on: test/screenshots is untracked so nothing automatic proved those, and snapshot -i stays proven by the fixture E2E tier.
Smaller notes (not inline):
uncoveredRunnerCacheInputs— the load-bearing safety argument for the whole skip — rests on a single-linehashFiles\(([^\n]+)\)regex (a reformat across lines or a secondhashFilesbreaks it) and asserts only cache-inputs ⊆ selection-inputs, so anything missing from both lists (pnpm-workspace.yaml,packages/*/package.json) is unprovable and the gap got worse post-PR (a missed build input now also skips, not just runs-vs-stale). Parse the action with theyamldep you already use inscripts/gate/workflows.ts, and assert the set both ways against one declared build-input list.changedPaths === null(diff failed) and[](nothing changed) share one reason string ("could not be established") that prints into the step summary — a genuinely empty change set reports as an infra failure. Keep both fail-open but give distinct reasons; that string is the operator's only audit trail for a skipped suite.- Two-dot vs three-dot: this gate uses
git diff baseSha HEADwhile the bridge proof uses"$BASE_SHA"...HEAD— two definitions of "changed" in one PR. Pick merge-base semantics in both, or share one change-set helper. - Entry guard deviates from siblings:
process.argv[1]?.endsWith('/ios-xctest-impact.ts')vs theimport.meta.url === pathToFileURL(...)idiom every other script inscripts/uses; and the test resolves workflow paths from CWD while its neighbour anchors onimport.meta.dirname.
| pnpm check:package -- --verify-snapshot-bridge-preparation | ||
|
|
||
| - name: Run targeted iOS runner XCTest regressions | ||
| if: steps.xctest-impact.outputs.run == 'true' |
There was a problem hiding this comment.
The module is written to fail open (non-PR / null / empty all return run: true), but this consumer fails closed: if: steps.xctest-impact.outputs.run == 'true' treats an absent output as skip. Rename the step id, move the step into a composite action or a separate selector job the way mutation-affected.yml does, or land a script edit that drops the output write, and you get '' == 'true' -> the whole ~110-test runner suite silently skipped with the job rendering green and no step summary at all. That is the one state where you have neither a test run nor a signal. Express the same decision as != 'false' so "no decision" is broad and the YAML agrees with the module's contract, and add a terminal assertion (in the macos.yml "reads as red rather than as a smaller green" style) that fails when the impact step reported a runner input changed but the XCTest step was skipped. Today the only thing between this lane and a silent full-suite skip is a hand-written regex over the workflow text.
There was a problem hiding this comment.
Fixed in 28a26fe7e8: the XCTest step now runs unless the selector explicitly outputs false. It has a step ID, and an if: always() assertion fails the job if selection reported true but the XCTest step was skipped. The generated -only-testing arguments also fail the step if generation fails or returns no tests.
| import fs from 'node:fs'; | ||
| import { spawnSync } from 'node:child_process'; | ||
|
|
||
| const RUNNER_INPUTS = [ |
There was a problem hiding this comment.
This is the repo's second hand-maintained answer to "which changed inputs own the iOS runner XCTest lane", and it is the one that ISN'T machine-enforced. scripts/check-affected/model.ts already declares path->check ownership (own:swift -> swift-runner-ios, own:xctest-selection) and check:gate-manifest / scripts/gate/routing.ts hold that model against the workflow paths-ignore lists, both ways, over every tracked path. RUNNER_INPUTS is exempt from that enforcement, so nothing keeps it complete: a new input that feeds the runner build (a script, a package path) added to the lane but missed here silently skips the XCTests it was supposed to gate — your fail-open promise is only as good as this list, and completeness is exactly what no gate checks here. AGENTS.md: don't add a guard that reconstructs another source of truth. Can this ask check-affected / selectChecks (or import an exported owner rule from it), so gate-manifest covers the step the way it covers the lane triggers, and docs/agents/testing.md points at the one declaration instead of restating the rule?
There was a problem hiding this comment.
Fixed in 28a26fe7e8: the separate RUNNER_INPUTS list is gone. scripts/apple-ci-impact.ts asks the shared selectChecks model whether swift-runner-ios is owned by the diff. I widened that owning rule for the Apple runner source subtree, and the gate manifest and tests enforce new paths. The PR XCTest method set is generated from Swift guards, so no test-name list needs updating either.
| - name: Setup toolchain | ||
| uses: ./.github/actions/setup-node-pnpm | ||
|
|
||
| - name: Verify clean-installed Simulator snapshot bridge preparation and the fold-helper -Werror gate |
There was a problem hiding this comment.
The PR correctly concludes that "should this proof run for this diff" belongs in a declared, tested input-set module — and then moves the other instance of that exact predicate into this job still written as an inline ~9-path git diff --quiet glob chain with a hand-tuned echo skip reason. Post-PR you have one predicate as a tested TS module and one as YAML shell globs, and the relocation makes the globs look canonical rather than pending; the two lists (bridge/fold sources, check-package.ts, size-report-*) will be edited by different people and drift. Migrate it the same way — one module exporting the proof's input set plus a fail-open select…, called as a one-line node step from both workflows, covered beside ios-xctest-impact.test.ts. Deleting the shell globs is the win here.
Two smaller things in this same step: the git fetch origin "$BASE_SHA" here is unguarded while the ios.yml copy has || echo '...fail open' — same command, opposite failure policy (with Actions' bash -eo pipefail a transient fetch failure reddens the whole macOS lane for an infra hiccup); and adding .github/workflows/ios.yml + macos.yml to a list whose skip branch prints "Snapshot bridge and fold-helper sources are unchanged" is self-referential and prints a wrong reason on a workflow-only change.
There was a problem hiding this comment.
Fixed in 28a26fe7e8: the inline glob chain is gone. The shared apple-ci-impact.ts selector handles bridge proof decisions, covers new apple/ and platform-Apple source paths, and fails open when fetching or reading the PR diff fails. The macOS proof runs after live replay, and its reason now names the actual selection outcome.
|
Reviewed at 08c10f3. The new commit only moves a step in The two questions from the last review are still open, because |
|
Resolved the review notes in |
|
This follow-up is on 28a26fe. The change-set detection this PR depends on is still broken, so the perf behavior it exists to deliver still cannot happen in CI. changedPathsFromGit in https://github.com/callstack/agent-device/blob/28a26fe/scripts/apple-ci-impact.ts#L53 runs a three-dot diff ( No CI run on any head has shown the skip route firing. The selection step at https://github.com/callstack/agent-device/blob/28a26fe/.github/workflows/ios.yml#L101, and the new "Assert iOS XCTest selection was honored" guard with SELECTED=false, have never executed against a real skip decision; the PR body also defers this to a later run. Once the change-set fix lands, this needs two runs: one on an unrelated diff where "Select iOS runner XCTests" logs "iOS XCTest: skip; the affected-check model selected no iOS runner build" and the XCTest step is skipped while the job stays green, and one on a runner-touching diff where the step logs "run; XCTest input changed: " and executes the generated 59 -only-testing ids. I did not run the vitest suites or the local gate at this head; I ran check-xctest-selection.ts --ios-pr-tests against a copy of the head tree only. The selector classifies a shared test by the guards in the test body, so could a shared test that reaches a production-side The iOS Smoke Tests job was still queued at 28a26fe, so there's no log to attribute there yet. macOS Smoke Tests passed at this head, but that pass is the bridge proof failing open described above, not a real exercise of the feature. This PR rewrites the ios.yml selection step, the generated -only-testing args, and the always() selection assert, so any failure in that job should be treated as likely related. There are no conflicts. The next thing needed is the change-set diff fix at scripts/apple-ci-impact.ts:53, followed by one CI run that actually takes the skip route. |
28a26fe to
0baea41
Compare
|
Confirmed. The macOS log's fail-open message came from using a three-dot diff in GitHub's shallow merge checkout. At I also traced the dropped shared-test case you raised. The rebased exact head passed |
|
The code looks ready at 0baea41. The follow-up on the earlier findings (#2896 (comment)) is resolved and nothing new turned up. Not blocking: you could open a throwaway draft PR based on this branch with an unrelated src/ change to confirm the skip route still logs "iOS XCTest: skip; the affected-check model selected no iOS runner build" and stays green, since this PR's own diff always takes the run route and the pull_request trigger has no branch filter — take it or leave it. I did not run the vitest suites (apple-ci-impact.test.ts, xctest-selection.test.ts) or the local gate at this head; I reproduced the shallow three-dot and two-dot behavior by hand with git. I reviewed only the logical delta: commit 0baea41 plus the range-diff hunk in a19fd96 that drops upstream-added ids from the hand-written list. A shared test with no guard in its own body that reaches a production #if os(iOS) branch would run only on macOS in PR CI; the nightly full iOS suite still covers it. I sampled about 8 of the 70 dropped tests (snapshot plan, private-AX pin, custom actions, raw plan, backend capabilities) and all were pure or took an explicit availableBackends or penalized argument — I did not trace the rest, and the scanner does not enforce the transitive case, as the PR description notes. The iOS Smoke Tests job (ios.yml smoke-ios) is still running. Its route overlaps this diff directly through the Select iOS runner XCTests step, the generated 64-id -only-testing list, and the always() selection assert, so a failure there should be treated as likely related to this PR. That run on 0baea41 needs to finish green while executing the generated 64-id -only-testing list before this can merge. |
thymikee
added
the
ready-for-human
|
|
Follow-up to the requested live skip proof: the disposable #2923 produced a successful iOS job on Measured runner time was 18m55s, versus 24m44s for #2896's successful exact-head iOS job: 5m49s saved in this pair. The probe spent extra time on retries (Settings passed on attempt 3; gesture on attempt 2), so this is one noisy sample, not a stable throughput claim. Queue time was 1m54s versus 29m01s and is reported separately. #2923 was rebased onto |
1 participant
Summary
Shorten selected iOS smoke runs by deriving their XCTest set from Swift guards and the shared affected-check model. The selector finds 64 iOS-specific tests on current
main; macOS covers shared host tests and nightly keeps the full iOS suite. Runner source cache inputs and test ownership are derived from source rather than maintained test-name lists. The macOS bridge proof runs after live UI replay and only when its owning Apple sources change.Use a two-tree Git diff so both selectors work in GitHub's shallow PR merge checkout. Remove three unasserted Settings captures and run the short pan replay before fixture E2E. This touches 15 files; no public API changes.
Validation
0baea4109cc79075ef2a7211d82bd3c4e8636bc5:pnpm check:affected --runpassed all runnable checks after rebasing onto merged #2901/#2902.pnpm check:xctest-selectionfound 316 declared methods: 64 in the iOS PR lane, 254 in the macOS host lane, 314 nightly, none unreachable. A shallow merge-shaped checkout test exercises the real CLI selector for both iOS XCTest and macOS bridge skip decisions; it failed before the two-tree fix and passes now. The earlier iOS smoke run passed on28a26fe, but its selector failed open. Exact-head CI is pending. A real GitHub skip path needs an unrelated PR diff after this selector lands.