From 1a9e5da51b2597eee14aa4102ce0a5243601fde0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 19:05:13 +0200 Subject: [PATCH 1/5] ci(ios): run the full XCTest suite nightly and check the PR test list (#1781 A7) --- .github/workflows/ci.yml | 9 + .github/workflows/xctest-nightly.yml | 151 +++++++++++++++++ docs/agents/testing.md | 1 + package.json | 3 +- scripts/__tests__/xctest-selection.test.ts | 188 +++++++++++++++++++++ scripts/check-affected/checks.ts | 6 + scripts/check-affected/model.ts | 11 ++ scripts/check-xctest-selection.ts | 170 +++++++++++++++++++ vitest.config.ts | 3 + 9 files changed, 541 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/xctest-nightly.yml create mode 100644 scripts/__tests__/xctest-selection.test.ts create mode 100644 scripts/check-xctest-selection.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 29979fed5..ee42fe5a0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -171,6 +171,15 @@ jobs: uses: ./.github/actions/run-gate with: { gate: gate-manifest } + # Same family as the manifest above — a CI selection that has stopped selecting what + # it claims. ios.yml runs 37 of the 153 runner XCTest methods through a hand-written + # `-only-testing:` list, and xcodebuild treats an identifier that matches nothing as + # an empty selection rather than an error, so a rename drops a test with no signal. + # Parse-only, no Xcode; the other 116 methods run in xctest-nightly.yml (#1781 A7). + - name: Check the PR XCTest selection still names real tests + uses: ./.github/actions/run-gate + with: { gate: xctest-selection } + maestro-conformance: name: Maestro Conformance Oracle runs-on: ubuntu-latest diff --git a/.github/workflows/xctest-nightly.yml b/.github/workflows/xctest-nightly.yml new file mode 100644 index 000000000..169aa4b5f --- /dev/null +++ b/.github/workflows/xctest-nightly.yml @@ -0,0 +1,151 @@ +name: XCTest Nightly + +# The full iOS runner XCTest suite (#1781 A7). The PR lane (ios.yml) runs 37 of the 153 +# `RunnerTests` methods through a hand-written `-only-testing:` list; before this lane the +# other 116 ran nowhere at all. This one drops the filter and runs the test plan whole. +# +# Under the lane rule in #1781: +# +# - Catches: regressions in the ~116 runner-semantics tests the PR list leaves out — +# selector matching, snapshot traversal, command journal, sequence execution, tv remote, +# keyboard, scroll/gesture policy. Nothing else executes them. +# - Evidence: the list has never been re-derived since it was written by hand; the tests it +# omits have been dark for their whole life, so their current state is unmeasured. The +# first red night is the evidence this lane exists to produce. +# - Cost: one macOS job per night. The Apple runner build is shared with ios.yml's cache +# (identical inputs and build variant), so a night that follows a green ios.yml run pays +# only the boot + full-suite time on top of a cache restore. Adds nothing to any PR. +# - Kill criterion: when A7's classification lands — pure Swift decision tests moved to a +# fast unit target, stale tests deleted, the rest kept on the simulator — this lane goes +# if the surviving set is small enough to run on every PR. +# +# Scheduled + manual only. It is deliberately NOT a PR gate: ios.yml is already over half of +# this repo's CI spend (#1781 A9), and adding 116 tests to it would make that worse for a set +# of tests whose flakiness on a shared runner is unknown. + +on: + schedule: + - cron: '30 4 * * *' + workflow_dispatch: + +permissions: + contents: read + actions: read + +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + full-suite: + name: iOS Runner Full XCTest Suite + runs-on: macos-26 + # Generous on purpose: the PR lane's 37 tests are a few minutes inside an 80-minute job + # that also runs replays and E2E, and this lane's total is unmeasured until it has run. + timeout-minutes: 120 + env: + # Must match ios.yml. The runtime version rides in the derived-data cache key, and + # AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS is BOTH an input to the cache's build-variant + # hash and the `-D AGENT_DEVICE_RUNNER_UNIT_TESTS` compile flag that makes the RunnerTests + # methods exist at all. Dropping it would build a different variant (cache miss) whose + # bundle contains no tests, and `xcodebuild` reports running zero tests as success — the + # "silent 0-test green" this lane would then be. The test-count assertion below is the + # backstop for exactly that. + IOS_RUNTIME_VERSION: '26.2' + AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS: '1' + AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH: ${{ github.workspace }}/.tmp/ios-runner-derived + RESULT_BUNDLE_PATH: ${{ github.workspace }}/.tmp/xctest-nightly/RunnerTests.xcresult + RESULT_SUMMARY_PATH: ${{ github.workspace }}/.tmp/xctest-nightly/test-results-summary.json + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Setup toolchain + uses: ./.github/actions/setup-node-pnpm + with: + cache-dependency-path: | + pnpm-lock.yaml + examples/test-app/pnpm-lock.yaml + + # Byte-identical inputs to ios.yml's step, so both lanes restore the same cache entry + # and only the first one to run on a commit pays for the build. + - name: Restore and build iOS XCTest runner + uses: ./.github/actions/setup-apple-runner-build + with: + derived-path: ${{ env.AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH }} + cache-key-prefix: ios-runner-prebuilt + cache-key-suffix: -ios-${{ env.IOS_RUNTIME_VERSION }} + gate: swift-runner-ios + xcuitest-platform: ios + xcuitest-destination: generic/platform=iOS Simulator + + - name: Boot iOS test simulator + id: ios-simulator + uses: ./.github/actions/boot-ios-test-simulator + with: + runtime-version: ${{ env.IOS_RUNTIME_VERSION }} + preferred-device-name: iPhone 17 Pro + + # Same command as ios.yml's targeted step minus every `-only-testing:` flag, so the + # xctestrun's own test plan decides what runs. + - name: Run the full iOS runner XCTest suite + run: | + set -euo pipefail + XCTESTRUN_PATH="$(find "$AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH/Build/Products" -maxdepth 1 -name '*.xctestrun' -print -quit)" + test -n "$XCTESTRUN_PATH" + mkdir -p "$(dirname "$RESULT_BUNDLE_PATH")" + xcodebuild test-without-building \ + -xctestrun "$XCTESTRUN_PATH" \ + -destination "platform=iOS Simulator,id=${{ steps.ios-simulator.outputs.simulator-udid }}" \ + -resultBundlePath "$RESULT_BUNDLE_PATH" + + # Runs even when the suite failed: a failing run is exactly when the per-test breakdown + # is worth having, and this step never changes the job's verdict on its own. + - name: Summarize the run + if: always() + run: | + set -uo pipefail + { + echo '### iOS runner full XCTest suite' + if [ -d "$RESULT_BUNDLE_PATH" ] && + xcrun xcresulttool get test-results summary --path "$RESULT_BUNDLE_PATH" \ + > "$RESULT_SUMMARY_PATH" 2>/dev/null; then + echo '```json' + cat "$RESULT_SUMMARY_PATH" + echo '```' + else + echo 'No result bundle summary was produced; see the run log and the uploaded bundle.' + fi + } >> "$GITHUB_STEP_SUMMARY" + + # The lane's own liveness check. `xcodebuild` exits 0 when a selection matches nothing, + # so "green" and "ran no tests" are the same log tail; a build variant without the + # unit-test compile flag, an empty test plan, or a renamed target would all read as a + # healthy night. Asserting the run executed tests is what tells those apart. + - name: Assert the suite actually ran tests + if: always() + run: | + set -euo pipefail + if [ ! -f "$RESULT_SUMMARY_PATH" ]; then + echo 'No test-results summary was produced, so nothing here can show a test ran.' + echo 'The suite step above failed before or during the run; read its log first.' + exit 1 + fi + TOTAL="$(RESULT_SUMMARY_PATH="$RESULT_SUMMARY_PATH" node -e ' + const summary = JSON.parse(require("node:fs").readFileSync(process.env.RESULT_SUMMARY_PATH, "utf8")); + process.stdout.write(String(summary.totalTestCount ?? 0)); + ')" + echo "Executed $TOTAL test(s)." + if [ "$TOTAL" -lt 1 ]; then + echo 'The full-suite lane executed no tests, which xcodebuild reports as success.' + echo 'Check AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS, the xctestrun test plan, and the target name.' + exit 1 + fi + + - name: Upload result bundle + if: always() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: xctest-nightly-results-${{ github.run_id }}-${{ github.run_attempt }} + path: .tmp/xctest-nightly + if-no-files-found: warn diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 0b060a062..746cbc3bc 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -37,6 +37,7 @@ The mapping it encodes, for when you need to run a gate directly or reason about | Platform/device response — anything emitting `platform`/`appleOs` on the wire, or shaping a daemon response | `pnpm test:integration:provider` **and** `pnpm test:coverage` | | Cross-platform behavior | `pnpm test:integration` | | Apple runner / Swift | Build the changed target with `pnpm build:xcuitest:`; use `pnpm build:xcuitest` only for shared iOS/macOS changes | +| Runner XCTest methods (`apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/**`) | `pnpm check:xctest-selection`. `ios.yml` runs 37 of the 153 methods through a hand-written `-only-testing:` list, and `xcodebuild` treats an identifier matching nothing as an empty selection rather than an error — so a rename drops a test silently. The check fails only on a listed name no source declares; a test that is in no list is expected, because `.github/workflows/xctest-nightly.yml` runs the suite whole every night | | CLI help/guidance (`src/cli/parser/cli-help.ts`, `src/cli-schema/`) | `pnpm exec vitest run src/cli/parser/__tests__ src/cli-schema/command-schema-guards.test.ts scripts/__tests__` — the `scripts/__tests__` gates enforce help-topic benchmark coverage and pin the bench's quoted CLI samples to the real renderers | | Help benchmark cases (`scripts/help-conformance-*.mjs`) | `pnpm exec vitest run scripts/__tests__` (deterministic gates); model-backed: `pnpm bench:help-conformance` (paid LLM calls, local only) | | `.ad` grammar (`src/replay/script.ts`, gesture arity, replay vars) | `pnpm exec vitest run --project unit-core test/replay-compat` — the frozen replay-compat corpus asserts which released script surfaces still parse; a flipped verdict is edited in `test/replay-compat/manifest.ts`, never in the script. Adding or re-pinning a corpus entry also runs `pnpm check:replay-compat`, which re-derives each entry from its release tag in git history | diff --git a/package.json b/package.json index ecc4a0ac3..34256fa23 100644 --- a/package.json +++ b/package.json @@ -143,6 +143,7 @@ "check:replay-compat": "node --experimental-strip-types scripts/check-replay-compat-provenance.ts", "check:daemon-wire-compat": "node --experimental-strip-types scripts/wire-compat/run.ts", "check:daemon-wire-compat:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/wire-compat/model.test.ts", + "check:xctest-selection": "node --experimental-strip-types scripts/check-xctest-selection.ts", "check:tmpdir-leaks": "node --experimental-strip-types scripts/check-tmpdir-leaks.ts", "check:tmpdir-leaks:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/check-tmpdir-leaks-model.test.ts scripts/vitest-tmpdir-global-setup.test.ts scripts/node-test-tmpdir.test.ts scripts/swift-toolchain-tmpdir.test.ts", "check:freerange": "fr", @@ -150,7 +151,7 @@ "sync:mcp-metadata": "node scripts/sync-mcp-metadata.mjs", "check:mcp-metadata": "node scripts/sync-mcp-metadata.mjs --check", "version": "pnpm sync:mcp-metadata && git add server.json", - "check:tooling": "pnpm format:check && pnpm lint && pnpm typecheck && pnpm check:layering && pnpm depgraph:test && pnpm check:gate-manifest:test && pnpm check:gate-manifest && pnpm check:production-exports && pnpm check:tmpdir-leaks:test && pnpm check:mcp-metadata && pnpm build && pnpm check:bundle-owner-files && pnpm check:package", + "check:tooling": "pnpm format:check && pnpm lint && pnpm typecheck && pnpm check:layering && pnpm depgraph:test && pnpm check:gate-manifest:test && pnpm check:gate-manifest && pnpm check:production-exports && pnpm check:tmpdir-leaks:test && pnpm check:xctest-selection && pnpm check:mcp-metadata && pnpm build && pnpm check:bundle-owner-files && pnpm check:package", "check:unit": "pnpm check:contention-retry && pnpm test:unit && pnpm check:tmpdir-leaks && pnpm test:smoke", "check": "pnpm check:tooling && pnpm check:fallow && pnpm check:unit", "prepack": "pnpm check:mcp-metadata && pnpm package:npm", diff --git a/scripts/__tests__/xctest-selection.test.ts b/scripts/__tests__/xctest-selection.test.ts new file mode 100644 index 000000000..b454c4f53 --- /dev/null +++ b/scripts/__tests__/xctest-selection.test.ts @@ -0,0 +1,188 @@ +// The check that keeps ios.yml's hand-written `-only-testing:` list honest is itself only +// as good as its two parsers, and both of its inputs are files nobody edits with this check +// in mind. So: the real tree must pass, and a planted typo in the real workflow text must +// fail. Synthetic sources cover the shapes the real tree happens not to contain today. + +import fs from 'node:fs'; +import path from 'node:path'; +import { describe, expect, test } from 'vitest'; +import { + buildReport, + formatSummary, + loadReport, + parseDeclaredTests, + parseSelectedTests, + PR_WORKFLOW_FILE, + reportFailures, + RUNNER_TESTS_DIR, + readSwiftSources, +} from '../check-xctest-selection.ts'; + +const repoRoot = path.resolve(import.meta.dirname, '..', '..'); +const TARGET = 'AgentDeviceRunnerUITests'; + +function source(text: string) { + return [{ file: 'RunnerTests+Fixture.swift', text }]; +} + +describe('the real tree', () => { + test('every `-only-testing:` entry in ios.yml names a declared test', () => { + expect(reportFailures(loadReport(repoRoot))).toEqual([]); + }); + + test('the PR lane selects a real subset — some tests run only in the nightly', () => { + const report = loadReport(repoRoot); + // Not pinned to today's 37/153: the point is that the list is a proper subset, so + // neither "the filter is gone" nor "the scan found nothing" reads as healthy. + expect(report.selected.length).toBeGreaterThan(0); + expect(report.declared.length).toBeGreaterThan(report.selected.length); + for (const entry of report.selected) { + expect(entry.identifier.startsWith(`${TARGET}/`)).toBe(true); + } + // The ratio is the whole point of the output — a passing run has to report it, or + // nobody reading CI logs can see the PR lane shrinking. + expect(formatSummary(report)).toContain( + `${report.selected.length} of ${report.declared.length}`, + ); + expect(formatSummary(report)).toContain( + `the other ${report.declared.length - report.selected.length} run in`, + ); + }); + + test('every declared test is addressable as the identifier a filter would use', () => { + // The count is the load-bearing claim: it must equal the `func test` methods in the + // sources, derived here the crude way the issue counted them. + const directory = path.join(repoRoot, RUNNER_TESTS_DIR); + const grepped = readSwiftSources(directory).reduce( + (total, entry) => total + (entry.text.match(/^ {2}(?:[\w@]+ )*func test/gm)?.length ?? 0), + 0, + ); + expect(loadReport(repoRoot).declared).toHaveLength(grepped); + }); +}); + +describe('a planted typo', () => { + test('a renamed test in the workflow list is reported with its line', () => { + const workflow = fs.readFileSync(path.join(repoRoot, PR_WORKFLOW_FILE), 'utf8'); + const first = parseSelectedTests(workflow)[0]; + if (!first) throw new Error('ios.yml has no -only-testing entries to plant a typo in'); + const typo = `${first.identifier}Renamed`; + const report = buildReport( + TARGET, + readSwiftSources(path.join(repoRoot, RUNNER_TESTS_DIR)), + workflow.replace(first.identifier, typo), + ); + + expect(report.unknown).toEqual([{ identifier: typo, line: first.line }]); + expect(reportFailures(report).join('\n')).toContain(typo); + }); + + test('a deleted test is reported even though the surviving list still passes', () => { + const swift = source( + 'extension RunnerTests {\n func testKept() {}\n func testGone() {}\n}\n', + ); + const workflow = [ + ` -only-testing:${TARGET}/RunnerTests/testKept \\`, + ` -only-testing:${TARGET}/RunnerTests/testGone`, + ].join('\n'); + + const before = buildReport(TARGET, swift, workflow); + expect(before.unknown).toEqual([]); + + const after = buildReport( + TARGET, + source('extension RunnerTests {\n func testKept() {}\n}\n'), + workflow, + ); + expect(after.unknown.map((entry) => entry.identifier)).toEqual([ + `${TARGET}/RunnerTests/testGone`, + ]); + }); +}); + +describe('the declaration scan', () => { + test('binds a method to the top-level type that encloses it', () => { + expect( + parseDeclaredTests( + TARGET, + source( + 'final class RunnerTests: XCTestCase {\n func testInClass() {}\n}\n\n' + + 'extension RunnerTests {\n func testInExtension() throws {}\n}\n\n' + + 'final class OtherTests: XCTestCase {\n func testElsewhere() async throws {}\n}\n', + ), + ), + ).toEqual([ + `${TARGET}/OtherTests/testElsewhere`, + `${TARGET}/RunnerTests/testInClass`, + `${TARGET}/RunnerTests/testInExtension`, + ]); + }); + + test('ignores declarations no filter could address', () => { + expect( + parseDeclaredTests( + TARGET, + source( + 'extension RunnerTests {\n' + + // A helper type declared inside a test body must not capture the methods after + // it, and `class func` must not read as a type declaration named `func`. + ' class func makeHelper() {}\n' + + ' func testWithNestedHelper() {\n' + + ' final class ResultBox {}\n' + + ' func testLocal() {}\n' + + ' }\n' + + ' // func testCommentedOut() {}\n' + + ' func testAfterNesting() {}\n' + + '}\n', + ), + ), + ).toEqual([ + `${TARGET}/RunnerTests/testAfterNesting`, + `${TARGET}/RunnerTests/testWithNestedHelper`, + ]); + }); +}); + +describe('the workflow scan', () => { + test('reads every entry on the multi-line xcodebuild invocation', () => { + expect( + parseSelectedTests( + [ + ' xcodebuild test-without-building \\', + ' -xctestrun "$XCTESTRUN_PATH" \\', + ` -only-testing:${TARGET}/RunnerTests/testOne \\`, + ` -only-testing:${TARGET}/RunnerTests/testTwo`, + ].join('\n'), + ), + ).toEqual([ + { identifier: `${TARGET}/RunnerTests/testOne`, line: 3 }, + { identifier: `${TARGET}/RunnerTests/testTwo`, line: 4 }, + ]); + }); + + test('leaves another target alone rather than guessing about sources it cannot see', () => { + const report = buildReport( + TARGET, + source('extension RunnerTests {\n func testOne() {}\n}\n'), + `-only-testing:SomeOtherTarget/OtherTests/testUnknown\n-only-testing:${TARGET}/RunnerTests/testOne`, + ); + expect(report.unknown).toEqual([]); + expect(report.selected).toHaveLength(2); + }); +}); + +describe('the blind-parse guards', () => { + test('an empty declaration scan fails instead of reporting a healthy list', () => { + const report = buildReport(TARGET, source('// nothing here\n'), '-only-testing:a/b/c'); + expect(reportFailures(report).join('\n')).toContain('declaration scan is broken'); + }); + + test('an empty workflow scan fails instead of reporting a healthy list', () => { + const report = buildReport( + TARGET, + source('extension RunnerTests {\n func testOne() {}\n}\n'), + '', + ); + expect(reportFailures(report).join('\n')).toContain('stopped filtering'); + }); +}); diff --git a/scripts/check-affected/checks.ts b/scripts/check-affected/checks.ts index a0d79f7ff..a3cc393ba 100644 --- a/scripts/check-affected/checks.ts +++ b/scripts/check-affected/checks.ts @@ -99,6 +99,12 @@ export const CHECK_CATALOG: readonly CheckSpec[] = [ gate('fixture-cache', 'Trusted fixture-artifact selection', 'test:fixture-cache'), gate('fixture-fallback', 'Fixture-app cache-failure fallback', 'test:fixture-fallback'), gate('command-docs', 'Command reference doc coverage', 'check:command-docs'), + // Parses ios.yml and the Swift sources; no Xcode, no simulator, so it runs anywhere. + gate( + 'xctest-selection', + 'PR XCTest `-only-testing:` list names tests that exist', + 'check:xctest-selection', + ), // --- Gates that drive their own runner ------------------------------------- // The ones no naming convention could find: an executable terminal for diff --git a/scripts/check-affected/model.ts b/scripts/check-affected/model.ts index a31f7ed21..5aab71233 100644 --- a/scripts/check-affected/model.ts +++ b/scripts/check-affected/model.ts @@ -67,6 +67,7 @@ export type CheckId = | 'fixture-cache' | 'fixture-fallback' | 'command-docs' + | 'xctest-selection' // Gates that drive their own runner — declared nowhere, registered here. | 'maestro-conformance' | 'maestro-differential' @@ -126,6 +127,7 @@ export const ALL_CHECKS: readonly CheckId[] = [ 'fixture-cache', 'fixture-fallback', 'command-docs', + 'xctest-selection', 'maestro-conformance', 'maestro-differential', 'maestro-regenerate', @@ -453,6 +455,15 @@ const BUILD_OWNERSHIP: ReadonlyArray<{ detail: 'Swift runner sources require the macOS XCUITest build', owns: (file) => file.startsWith('apple/runner/') || file.endsWith('.swift'), }, + // The PR lane names each runner XCTest method it runs, so renaming or deleting one + // silently shrinks that lane. Selected here so the drift shows up on the change that + // causes it rather than on the next nightly. + { + check: 'xctest-selection', + rule: 'own:xctest-selection', + detail: "the PR lane's hand-written `-only-testing:` list names these methods", + owns: (file) => file.startsWith('apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/'), + }, { check: 'android-helpers', rule: 'own:android-helpers', diff --git a/scripts/check-xctest-selection.ts b/scripts/check-xctest-selection.ts new file mode 100644 index 000000000..d850725a1 --- /dev/null +++ b/scripts/check-xctest-selection.ts @@ -0,0 +1,170 @@ +// `pnpm check:xctest-selection` — hold the hand-written `-only-testing:` list in +// .github/workflows/ios.yml to the tests that actually exist (#1781 A7). +// +// The PR lane runs a subset of the runner XCTest suite by naming each method on the +// xcodebuild command line. `xcodebuild test-without-building` treats an +// `-only-testing:` identifier that matches nothing as an empty selection rather than an +// error, so a renamed or deleted test does not fail the lane — it silently stops being +// tested, and the lane stays green with fewer tests than the list claims. That failure +// mode is invisible in logs unless someone counts, which is what this check does. +// +// Deliberately one-directional: a test that exists but is NOT in the PR list is fine, it +// runs in the nightly full-suite lane (.github/workflows/xctest-nightly.yml). Only the +// reverse — a listed name no source declares — is a defect. +// +// The declaration scan is source-level, so a method compiled out by an `#if os(...)` +// guard still counts as declared. That is the intended precision: this check guards the +// list against renames and deletions, not against platform availability, which the +// nightly full-suite run observes directly. + +import fs from 'node:fs'; +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; + +const repoRoot = path.resolve(import.meta.dirname, '..'); + +/** The XCTest target directory; its basename is the target name the identifiers use. */ +export const RUNNER_TESTS_DIR = 'apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests'; + +/** The workflow whose `-only-testing:` list this check guards. */ +export const PR_WORKFLOW_FILE = '.github/workflows/ios.yml'; + +/** The workflow that runs everything the PR list leaves out. */ +const NIGHTLY_WORKFLOW_FILE = '.github/workflows/xctest-nightly.yml'; + +const RUNNER_TESTS_SOURCE = /^RunnerTests.*\.swift$/; + +// One ordered pass over the source. A column-0 type declaration moves the enclosing type; +// a `func test…` indented exactly one level binds to it. Position carries the meaning +// rather than brace counting, which would have to know which `{` sits inside a string +// literal. It is also the more precise rule: only a method declared directly in a +// top-level `class`/`extension` block is addressable as `Target/Class/method`, so a +// helper type nested inside a test body (`final class ResultBox` — several of these +// exist) contributes no test identifiers, and neither does a closure-local `func test…`. +const DECLARATION = + /^(?:[\w@]+[ \t]+)*(?:class|extension|struct|enum|actor|protocol)[ \t]+([A-Za-z_]\w*)|^ {2}(?:[\w@]+[ \t]+)*func[ \t]+(test\w*)[ \t]*\(/gm; + +const ONLY_TESTING = /-only-testing:(\S+)/; + +export type SwiftSource = { readonly file: string; readonly text: string }; + +/** One `-only-testing:` identifier as written, with the line that carries it. */ +export type SelectedTest = { readonly identifier: string; readonly line: number }; + +export type SelectionReport = { + readonly target: string; + /** Every `Target/Class/method` the Swift sources declare, sorted. */ + readonly declared: readonly string[]; + /** Every `-only-testing:` identifier in the PR workflow, in file order. */ + readonly selected: readonly SelectedTest[]; + /** Selected identifiers naming no declared method — the failure. */ + readonly unknown: readonly SelectedTest[]; +}; + +export function readSwiftSources(directory: string): SwiftSource[] { + return fs + .readdirSync(directory) + .filter((entry) => RUNNER_TESTS_SOURCE.test(entry)) + .sort() + .map((entry) => ({ + file: entry, + text: fs.readFileSync(path.join(directory, entry), 'utf8'), + })); +} + +/** Every `Target/Class/method` identifier the sources declare. */ +export function parseDeclaredTests(target: string, sources: readonly SwiftSource[]): string[] { + const declared = new Set(); + for (const source of sources) { + let enclosing = ''; + for (const [, type, method] of source.text.matchAll(DECLARATION)) { + if (type !== undefined) enclosing = type; + else if (method !== undefined && enclosing) declared.add(`${target}/${enclosing}/${method}`); + } + } + return [...declared].sort(); +} + +/** Every `-only-testing:` identifier the workflow names, with its line number. */ +export function parseSelectedTests(workflowText: string): SelectedTest[] { + return workflowText.split('\n').flatMap((text, index) => { + const identifier = ONLY_TESTING.exec(text)?.[1]; + return identifier ? [{ identifier, line: index + 1 }] : []; + }); +} + +export function buildReport( + target: string, + sources: readonly SwiftSource[], + workflowText: string, +): SelectionReport { + const declared = parseDeclaredTests(target, sources); + const known = new Set(declared); + const selected = parseSelectedTests(workflowText); + return { + target, + declared, + selected, + // Identifiers for another target are left alone: this check owns one target's + // sources and cannot speak for anything else the workflow might select. + unknown: selected.filter( + (entry) => entry.identifier.startsWith(`${target}/`) && !known.has(entry.identifier), + ), + }; +} + +export function loadReport(root: string = repoRoot): SelectionReport { + const directory = path.join(root, RUNNER_TESTS_DIR); + return buildReport( + path.basename(directory), + readSwiftSources(directory), + fs.readFileSync(path.join(root, PR_WORKFLOW_FILE), 'utf8'), + ); +} + +/** The failures, or an empty list. Kept separate from formatting so the test can assert both. */ +export function reportFailures(report: SelectionReport): string[] { + // A parser that stops matching would report "0 unknown" forever, which reads exactly + // like a healthy list. Both inputs are non-empty by construction, so an empty parse is + // the check going blind, not a real state of the tree. + if (report.declared.length === 0) { + return [ + `Found no test methods in ${RUNNER_TESTS_DIR}. The declaration scan is broken, ` + + 'so this check can no longer see a dropped test.', + ]; + } + if (report.selected.length === 0) { + return [ + `Found no \`-only-testing:\` entries in ${PR_WORKFLOW_FILE}. Either the PR lane stopped ` + + 'filtering (drop this check), or the scan is broken and can no longer see a dropped test.', + ]; + } + if (report.unknown.length === 0) return []; + return [ + `${PR_WORKFLOW_FILE} selects ${report.unknown.length} XCTest method(s) that no source declares:`, + ...report.unknown.map((entry) => ` - line ${entry.line}: ${entry.identifier}`), + 'xcodebuild runs nothing for an unmatched `-only-testing:` identifier and still exits 0, so a', + `rename or deletion drops the test silently. Update the entry to the current name, or remove`, + `it — ${NIGHTLY_WORKFLOW_FILE} runs the whole suite either way.`, + ]; +} + +export function formatSummary(report: SelectionReport): string { + const remaining = report.declared.length - report.selected.length; + return ( + `xctest selection: ${report.selected.length} of ${report.declared.length} ${report.target} ` + + `methods run on every PR (${PR_WORKFLOW_FILE}); the other ${remaining} run in ` + + `${NIGHTLY_WORKFLOW_FILE}.\n` + ); +} + +function main(): number { + const report = loadReport(); + const failures = reportFailures(report); + process.stdout.write(formatSummary(report)); + if (failures.length === 0) return 0; + process.stderr.write(`${failures.join('\n')}\n`); + return 1; +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) process.exit(main()); diff --git a/vitest.config.ts b/vitest.config.ts index 4c922b100..cfe7b7cde 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -77,6 +77,9 @@ export default defineConfig({ 'scripts/__tests__/agent-setup-startup-contract.test.ts', 'scripts/__tests__/npm-skills-exclusion.test.ts', 'scripts/__tests__/simulator-skills-contract.test.ts', + // Parses ios.yml and the runner's Swift sources: no Xcode, no simulator, and + // the check it guards is what keeps the PR lane's `-only-testing:` list honest. + 'scripts/__tests__/xctest-selection.test.ts', // The Fallow fixture policy is executable configuration: unused exports are exempt, // but fixture modules remain visible to the other analysis families. 'scripts/__tests__/fallow-fixture-policy.test.ts', From 353c827686a00258a9d5af04d4c3aa71f75869bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 19:28:36 +0200 Subject: [PATCH 2/5] fix(ci): skip the runner server entry point in the nightly and validate both test flags --- .github/workflows/ci.yml | 7 +- .github/workflows/xctest-nightly.yml | 91 ++++--- docs/agents/testing.md | 2 +- scripts/__tests__/xctest-run-summary.test.ts | 117 +++++++++ scripts/__tests__/xctest-selection.test.ts | 260 ++++++++++++++----- scripts/check-affected/model.test.ts | 18 ++ scripts/check-xctest-selection.ts | 186 +++++++++---- scripts/xctest-run-summary.ts | 100 +++++++ vitest.config.ts | 3 + 9 files changed, 625 insertions(+), 159 deletions(-) create mode 100644 scripts/__tests__/xctest-run-summary.test.ts create mode 100644 scripts/xctest-run-summary.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ee42fe5a0..958450578 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -172,10 +172,11 @@ jobs: with: { gate: gate-manifest } # Same family as the manifest above — a CI selection that has stopped selecting what - # it claims. ios.yml runs 37 of the 153 runner XCTest methods through a hand-written + # it claims. ios.yml runs 37 of the 154 runner XCTest methods through a hand-written # `-only-testing:` list, and xcodebuild treats an identifier that matches nothing as - # an empty selection rather than an error, so a rename drops a test with no signal. - # Parse-only, no Xcode; the other 116 methods run in xctest-nightly.yml (#1781 A7). + # an empty selection rather than an error, so a rename drops a test with no signal — + # in both directions, since a typo in xctest-nightly.yml's `-skip-testing:` entry + # re-arms a 24-hour hang. Parse-only, no Xcode (#1781 A7). - name: Check the PR XCTest selection still names real tests uses: ./.github/actions/run-gate with: { gate: xctest-selection } diff --git a/.github/workflows/xctest-nightly.yml b/.github/workflows/xctest-nightly.yml index 169aa4b5f..332202ac1 100644 --- a/.github/workflows/xctest-nightly.yml +++ b/.github/workflows/xctest-nightly.yml @@ -1,14 +1,18 @@ name: XCTest Nightly -# The full iOS runner XCTest suite (#1781 A7). The PR lane (ios.yml) runs 37 of the 153 -# `RunnerTests` methods through a hand-written `-only-testing:` list; before this lane the -# other 116 ran nowhere at all. This one drops the filter and runs the test plan whole. +# The full iOS runner XCTest suite (#1781 A7). The target declares 154 addressable methods; +# the PR lane (ios.yml) names 37 of them in a hand-written `-only-testing:` list, so 117 ran +# nowhere at all. This lane drops that filter and runs the test plan whole, skipping only +# `testCommand` — which is the runner's server entry point rather than a test (see the step +# below) — for 116 methods that nothing else executes. # # Under the lane rule in #1781: # -# - Catches: regressions in the ~116 runner-semantics tests the PR list leaves out — +# - Catches: regressions in the 116 runner-semantics tests the PR list leaves out — # selector matching, snapshot traversal, command journal, sequence execution, tv remote, -# keyboard, scroll/gesture policy. Nothing else executes them. +# keyboard, scroll/gesture policy. Nothing else executes them. Two of the 154 +# (`…OnTvOS`) are behind `#if os(tvOS)` and so never execute on this lane's destination; +# the honest iOS-reachable figure is 151. # - Evidence: the list has never been re-derived since it was written by hand; the tests it # omits have been dark for their whole life, so their current state is unmeasured. The # first red night is the evidence this lane exists to produce. @@ -22,6 +26,11 @@ name: XCTest Nightly # Scheduled + manual only. It is deliberately NOT a PR gate: ios.yml is already over half of # this repo's CI spend (#1781 A9), and adding 116 tests to it would make that worse for a set # of tests whose flakiness on a shared runner is unknown. +# +# Note on `concurrency` below: it is keyed on `github.ref`, so a manual dispatch on the same +# ref cancels an in-flight scheduled run. That is the intended trade — an operator asking for +# a run wants the fresh one — but it means "dispatch while the nightly is running" loses the +# nightly's result rather than queueing behind it. on: schedule: @@ -87,7 +96,21 @@ jobs: preferred-device-name: iPhone 17 Pro # Same command as ios.yml's targeted step minus every `-only-testing:` flag, so the - # xctestrun's own test plan decides what runs. + # xctestrun's own test plan decides what runs — with one exception. + # + # `RunnerTests/testCommand` is not a test. It is the runner's server entry point: it + # opens an NWListener and blocks in `XCTWaiter.wait(timeout: 24 * 60 * 60)` until a + # client closes the session, which is exactly how production drives it + # (src/platforms/apple/core/runner/runner-session.ts always passes it as the sole + # `-only-testing:`). It compiles unconditionally — the `#if AGENT_DEVICE_RUNNER_UNIT_TESTS` + # block ends well above it — so an unfiltered run reaches it in alphabetical order and + # hangs the job until `timeout-minutes`. Its one escape hatch, + # AGENT_DEVICE_RUNNER_NOOP_STARTUP, is an environment variable, and the comment above + # the flag records that env plumbing into a simulator test process is not dependable; + # `-skip-testing:` is the lever that works from here. + # + # A typo in that identifier silently re-arms the hang, so + # `pnpm check:xctest-selection` validates `-skip-testing:` exactly like `-only-testing:`. - name: Run the full iOS runner XCTest suite run: | set -euo pipefail @@ -97,50 +120,42 @@ jobs: xcodebuild test-without-building \ -xctestrun "$XCTESTRUN_PATH" \ -destination "platform=iOS Simulator,id=${{ steps.ios-simulator.outputs.simulator-udid }}" \ + -skip-testing:AgentDeviceRunnerUITests/RunnerTests/testCommand \ -resultBundlePath "$RESULT_BUNDLE_PATH" - # Runs even when the suite failed: a failing run is exactly when the per-test breakdown - # is worth having, and this step never changes the job's verdict on its own. - - name: Summarize the run + # Best-effort and never the job's verdict on its own; the step below is what asserts. + # `--compact` first because a red night's summary is the large one, and the job summary + # has a 1 MiB cap — the plain form is the fallback if an Xcode version rejects the flag. + - name: Extract the test-results summary if: always() run: | set -uo pipefail - { - echo '### iOS runner full XCTest suite' - if [ -d "$RESULT_BUNDLE_PATH" ] && - xcrun xcresulttool get test-results summary --path "$RESULT_BUNDLE_PATH" \ - > "$RESULT_SUMMARY_PATH" 2>/dev/null; then - echo '```json' - cat "$RESULT_SUMMARY_PATH" - echo '```' - else - echo 'No result bundle summary was produced; see the run log and the uploaded bundle.' - fi - } >> "$GITHUB_STEP_SUMMARY" + [ -d "$RESULT_BUNDLE_PATH" ] || exit 0 + xcrun xcresulttool get test-results summary --path "$RESULT_BUNDLE_PATH" --compact \ + > "$RESULT_SUMMARY_PATH" 2>/dev/null || + xcrun xcresulttool get test-results summary --path "$RESULT_BUNDLE_PATH" \ + > "$RESULT_SUMMARY_PATH" 2>/dev/null || + rm -f "$RESULT_SUMMARY_PATH" + exit 0 - # The lane's own liveness check. `xcodebuild` exits 0 when a selection matches nothing, - # so "green" and "ran no tests" are the same log tail; a build variant without the - # unit-test compile flag, an empty test plan, or a renamed target would all read as a - # healthy night. Asserting the run executed tests is what tells those apart. - - name: Assert the suite actually ran tests + # Reports the run AND is the lane's own liveness check. `xcodebuild` exits 0 when a + # selection matches nothing, so "green" and "ran no tests" are the same log tail: a + # build variant without the unit-test compile flag, an empty test plan, or a renamed + # target would all read as a healthy night. Asserting the run executed tests is what + # tells those apart. The failure list is capped so a badly red night cannot blow the + # 1 MiB job-summary limit and lose the headline with it. + - name: Report the run and assert it executed tests if: always() run: | set -euo pipefail - if [ ! -f "$RESULT_SUMMARY_PATH" ]; then - echo 'No test-results summary was produced, so nothing here can show a test ran.' + if [ ! -s "$RESULT_SUMMARY_PATH" ]; then + echo 'No usable test-results summary was produced, so nothing here can show a test ran.' echo 'The suite step above failed before or during the run; read its log first.' + echo '### iOS runner full XCTest suite' >> "$GITHUB_STEP_SUMMARY" + echo 'No result-bundle summary; see the run log and the uploaded bundle.' >> "$GITHUB_STEP_SUMMARY" exit 1 fi - TOTAL="$(RESULT_SUMMARY_PATH="$RESULT_SUMMARY_PATH" node -e ' - const summary = JSON.parse(require("node:fs").readFileSync(process.env.RESULT_SUMMARY_PATH, "utf8")); - process.stdout.write(String(summary.totalTestCount ?? 0)); - ')" - echo "Executed $TOTAL test(s)." - if [ "$TOTAL" -lt 1 ]; then - echo 'The full-suite lane executed no tests, which xcodebuild reports as success.' - echo 'Check AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS, the xctestrun test plan, and the target name.' - exit 1 - fi + node --experimental-strip-types scripts/xctest-run-summary.ts - name: Upload result bundle if: always() diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 746cbc3bc..fad8b07d7 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -37,7 +37,7 @@ The mapping it encodes, for when you need to run a gate directly or reason about | Platform/device response — anything emitting `platform`/`appleOs` on the wire, or shaping a daemon response | `pnpm test:integration:provider` **and** `pnpm test:coverage` | | Cross-platform behavior | `pnpm test:integration` | | Apple runner / Swift | Build the changed target with `pnpm build:xcuitest:`; use `pnpm build:xcuitest` only for shared iOS/macOS changes | -| Runner XCTest methods (`apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/**`) | `pnpm check:xctest-selection`. `ios.yml` runs 37 of the 153 methods through a hand-written `-only-testing:` list, and `xcodebuild` treats an identifier matching nothing as an empty selection rather than an error — so a rename drops a test silently. The check fails only on a listed name no source declares; a test that is in no list is expected, because `.github/workflows/xctest-nightly.yml` runs the suite whole every night | +| Runner XCTest methods (`apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/**`) | `pnpm check:xctest-selection`. `ios.yml` runs 37 of the 154 methods through a hand-written `-only-testing:` list and `xctest-nightly.yml` excludes one via `-skip-testing:`; `xcodebuild` treats an identifier matching nothing as an empty selection rather than an error, in both directions — a rename drops a test from the PR lane silently, or re-admits `RunnerTests/testCommand`, the runner's 24-hour server entry point, into the nightly and hangs it. The check fails only on a listed name no source declares; a test in no list is expected, because the nightly runs the suite whole | | CLI help/guidance (`src/cli/parser/cli-help.ts`, `src/cli-schema/`) | `pnpm exec vitest run src/cli/parser/__tests__ src/cli-schema/command-schema-guards.test.ts scripts/__tests__` — the `scripts/__tests__` gates enforce help-topic benchmark coverage and pin the bench's quoted CLI samples to the real renderers | | Help benchmark cases (`scripts/help-conformance-*.mjs`) | `pnpm exec vitest run scripts/__tests__` (deterministic gates); model-backed: `pnpm bench:help-conformance` (paid LLM calls, local only) | | `.ad` grammar (`src/replay/script.ts`, gesture arity, replay vars) | `pnpm exec vitest run --project unit-core test/replay-compat` — the frozen replay-compat corpus asserts which released script surfaces still parse; a flipped verdict is edited in `test/replay-compat/manifest.ts`, never in the script. Adding or re-pinning a corpus entry also runs `pnpm check:replay-compat`, which re-derives each entry from its release tag in git history | diff --git a/scripts/__tests__/xctest-run-summary.test.ts b/scripts/__tests__/xctest-run-summary.test.ts new file mode 100644 index 000000000..51987bac7 --- /dev/null +++ b/scripts/__tests__/xctest-run-summary.test.ts @@ -0,0 +1,117 @@ +// The nightly XCTest lane's reporter is also its liveness check, and both halves only ever +// execute on a macOS runner at 04:30 — so they are proven here instead. + +import { describe, expect, test } from 'vitest'; +import { + livenessFailure, + MAX_FAILURE_TEXT, + MAX_LISTED_FAILURES, + renderSummary, + type ResultSummary, +} from '../xctest-run-summary.ts'; + +function failures(count: number, text = 'assertion failed') { + return Array.from({ length: count }, (_, index) => ({ + testName: `testCase${index}()`, + failureText: text, + })); +} + +describe('the liveness check', () => { + test('a run that executed no tests fails, because xcodebuild calls that success', () => { + // The lane's whole reason to assert: a build without the unit-test compile flag, an + // empty test plan, or a `-skip-testing:` entry that swallowed the suite all exit 0. + const failure = livenessFailure({ result: 'Passed', totalTestCount: 0 }); + expect(failure).toContain('executed no tests'); + expect(failure).toContain('AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS'); + expect(failure).toContain('-skip-testing'); + }); + + test('a summary missing the count entirely is treated as no tests, not as unknown', () => { + expect(livenessFailure({})).not.toBeNull(); + }); + + test('a run that executed tests passes, red or green', () => { + expect(livenessFailure({ totalTestCount: 153, result: 'Failed', failedTests: 9 })).toBeNull(); + expect(livenessFailure({ totalTestCount: 1, result: 'Passed' })).toBeNull(); + }); +}); + +describe('the job summary', () => { + const green: ResultSummary = { + result: 'Passed', + totalTestCount: 153, + passedTests: 153, + failedTests: 0, + skippedTests: 0, + expectedFailures: 0, + startTime: 1000, + finishTime: 1450, + }; + + test('leads with the headline a reader needs', () => { + const rendered = renderSummary(green); + expect(rendered).toContain('- result: **Passed**'); + expect(rendered).toContain('- executed: **153** (passed 153, failed 0, skipped 0'); + expect(rendered).toContain('- duration: 450s'); + expect(rendered).not.toContain('#### Failures'); + }); + + test('reports an unknown duration rather than NaN when the times are absent', () => { + expect(renderSummary({ totalTestCount: 1 })).toContain('- duration: unknown'); + }); + + test('lists failures and names each one', () => { + const rendered = renderSummary({ ...green, result: 'Failed', testFailures: failures(3) }); + expect(rendered).toContain('#### Failures'); + expect(rendered).toContain('`testCase0()`'); + expect(rendered).toContain('assertion failed'); + }); + + test('caps the list, and says how many it dropped', () => { + // The night this lane matters most is the night the failure list is longest, and the + // job summary is capped at 1 MiB — losing the headline to the tail would be the worst + // possible trade. + const rendered = renderSummary({ + ...green, + result: 'Failed', + testFailures: failures(MAX_LISTED_FAILURES + 7), + }); + const listed = rendered.split('\n').filter((line) => line.startsWith('- `test')).length; + expect(listed).toBe(MAX_LISTED_FAILURES); + expect(rendered).toContain('…and 7 more'); + }); + + test('truncates one enormous failure message instead of letting it dominate', () => { + const rendered = renderSummary({ + ...green, + result: 'Failed', + testFailures: failures(1, 'x'.repeat(5000)), + }); + expect(rendered).toContain('x'.repeat(MAX_FAILURE_TEXT)); + expect(rendered).not.toContain('x'.repeat(MAX_FAILURE_TEXT + 1)); + }); + + test('flattens newlines so a multi-line stack cannot forge markdown structure', () => { + const rendered = renderSummary({ + ...green, + result: 'Failed', + testFailures: [{ testName: 'testX()', failureText: 'line one\n#### Injected\nline two' }], + }); + // Markdown headings only bind at the start of a line, so flattening is what disarms + // the injection: the text survives verbatim, but it can no longer open a section. + expect(rendered.split('\n').filter((line) => line.startsWith('#'))).toEqual([ + '### iOS runner full XCTest suite', + '#### Failures', + ]); + expect(rendered).toContain('line one #### Injected line two'); + }); + + test('falls back to the identifier when a failure carries no test name', () => { + const rendered = renderSummary({ + ...green, + testFailures: [{ testIdentifierString: 'RunnerTests/testY()', failureText: 'boom' }], + }); + expect(rendered).toContain('`RunnerTests/testY()`'); + }); +}); diff --git a/scripts/__tests__/xctest-selection.test.ts b/scripts/__tests__/xctest-selection.test.ts index b454c4f53..731589fa5 100644 --- a/scripts/__tests__/xctest-selection.test.ts +++ b/scripts/__tests__/xctest-selection.test.ts @@ -1,21 +1,27 @@ -// The check that keeps ios.yml's hand-written `-only-testing:` list honest is itself only -// as good as its two parsers, and both of its inputs are files nobody edits with this check -// in mind. So: the real tree must pass, and a planted typo in the real workflow text must -// fail. Synthetic sources cover the shapes the real tree happens not to contain today. +// The check that keeps the iOS workflows' hand-written test identifiers honest is itself +// only as good as its two parsers, and both of its inputs are files nobody edits with this +// check in mind. So: the real tree must pass, and a planted typo in the real workflow text +// must fail — in both directions, because an unknown `-skip-testing:` entry re-arms the +// nightly's 24-hour hang on `RunnerTests/testCommand`. Synthetic sources cover the shapes +// the real tree happens not to contain today. import fs from 'node:fs'; import path from 'node:path'; import { describe, expect, test } from 'vitest'; import { buildReport, + counts, formatSummary, + GUARDED_WORKFLOWS, loadReport, + NIGHTLY_WORKFLOW_FILE, parseDeclaredTests, - parseSelectedTests, + parseFlaggedTests, PR_WORKFLOW_FILE, + readSwiftSources, reportFailures, RUNNER_TESTS_DIR, - readSwiftSources, + type WorkflowSource, } from '../check-xctest-selection.ts'; const repoRoot = path.resolve(import.meta.dirname, '..', '..'); @@ -25,78 +31,124 @@ function source(text: string) { return [{ file: 'RunnerTests+Fixture.swift', text }]; } +function realWorkflows(overrides: Readonly> = {}): WorkflowSource[] { + return GUARDED_WORKFLOWS.map((workflow) => ({ + workflow, + text: overrides[workflow] ?? fs.readFileSync(path.join(repoRoot, workflow), 'utf8'), + })); +} + +function realSources() { + return readSwiftSources(path.join(repoRoot, RUNNER_TESTS_DIR)); +} + describe('the real tree', () => { - test('every `-only-testing:` entry in ios.yml names a declared test', () => { + test('every flagged identifier in both workflows names a declared test', () => { expect(reportFailures(loadReport(repoRoot))).toEqual([]); }); - test('the PR lane selects a real subset — some tests run only in the nightly', () => { + test('the PR lane selects a real subset — most tests are reached only by the nightly', () => { const report = loadReport(repoRoot); - // Not pinned to today's 37/153: the point is that the list is a proper subset, so - // neither "the filter is gone" nor "the scan found nothing" reads as healthy. - expect(report.selected.length).toBeGreaterThan(0); - expect(report.declared.length).toBeGreaterThan(report.selected.length); - for (const entry of report.selected) { - expect(entry.identifier.startsWith(`${TARGET}/`)).toBe(true); - } - // The ratio is the whole point of the output — a passing run has to report it, or - // nobody reading CI logs can see the PR lane shrinking. - expect(formatSummary(report)).toContain( - `${report.selected.length} of ${report.declared.length}`, - ); - expect(formatSummary(report)).toContain( - `the other ${report.declared.length - report.selected.length} run in`, - ); + const { declared, pr, skipped, nightlyOnly } = counts(report); + // Not pinned to today's exact numbers; the invariants are that the PR list is a proper + // subset, that the nightly skips something, and that the three partition the suite. + expect(pr).toBeGreaterThan(0); + expect(skipped).toBeGreaterThan(0); + expect(declared).toBeGreaterThan(pr + skipped); + expect(pr + skipped + nightlyOnly).toBe(declared); + }); + + test('the nightly skips the runner server entry point, which is not a test', () => { + // The whole reason -skip-testing: exists in this repo. `testCommand` opens an + // NWListener and waits 24 hours; an unfiltered run would hang the lane to its timeout. + const skipped = loadReport(repoRoot).flagged.filter((entry) => entry.flag === 'skip-testing'); + expect(skipped.map((entry) => entry.identifier)).toContain(`${TARGET}/RunnerTests/testCommand`); + for (const entry of skipped) expect(entry.workflow).toBe(NIGHTLY_WORKFLOW_FILE); }); - test('every declared test is addressable as the identifier a filter would use', () => { - // The count is the load-bearing claim: it must equal the `func test` methods in the - // sources, derived here the crude way the issue counted them. + test('the declared set covers every addressable method in the target directory', () => { + // Derived independently of the check: the directory is globbed here, with this test's + // own regex, because the Xcode project uses a PBXFileSystemSynchronizedRootGroup — every + // .swift file in it is a member. Reusing the check's own file filter would make this + // tautological, and a name-based filter is exactly the bug it caught + // (RunnerTapPointPolicy.swift declares a test and does not start with "RunnerTests"). const directory = path.join(repoRoot, RUNNER_TESTS_DIR); - const grepped = readSwiftSources(directory).reduce( - (total, entry) => total + (entry.text.match(/^ {2}(?:[\w@]+ )*func test/gm)?.length ?? 0), - 0, - ); - expect(loadReport(repoRoot).declared).toHaveLength(grepped); + const counted = fs + .readdirSync(directory) + .filter((entry) => entry.endsWith('.swift')) + .reduce((total, entry) => { + const text = fs.readFileSync(path.join(directory, entry), 'utf8'); + return total + (text.match(/^ {2}(?:[\w@]+ )*func test/gm)?.length ?? 0); + }, 0); + + expect(counted).toBeGreaterThan(0); + expect(loadReport(repoRoot).declared).toHaveLength(counted); }); }); describe('a planted typo', () => { - test('a renamed test in the workflow list is reported with its line', () => { + test('a renamed test in the PR `-only-testing:` list is reported with its line', () => { const workflow = fs.readFileSync(path.join(repoRoot, PR_WORKFLOW_FILE), 'utf8'); - const first = parseSelectedTests(workflow)[0]; + const first = parseFlaggedTests(PR_WORKFLOW_FILE, workflow).find( + (entry) => entry.flag === 'only-testing', + ); if (!first) throw new Error('ios.yml has no -only-testing entries to plant a typo in'); const typo = `${first.identifier}Renamed`; + const report = buildReport( TARGET, - readSwiftSources(path.join(repoRoot, RUNNER_TESTS_DIR)), - workflow.replace(first.identifier, typo), + realSources(), + realWorkflows({ [PR_WORKFLOW_FILE]: workflow.replace(first.identifier, typo) }), ); - expect(report.unknown).toEqual([{ identifier: typo, line: first.line }]); + expect(report.unknown).toEqual([ + { workflow: PR_WORKFLOW_FILE, flag: 'only-testing', identifier: typo, line: first.line }, + ]); expect(reportFailures(report).join('\n')).toContain(typo); }); - test('a deleted test is reported even though the surviving list still passes', () => { - const swift = source( - 'extension RunnerTests {\n func testKept() {}\n func testGone() {}\n}\n', - ); - const workflow = [ - ` -only-testing:${TARGET}/RunnerTests/testKept \\`, - ` -only-testing:${TARGET}/RunnerTests/testGone`, - ].join('\n'); - - const before = buildReport(TARGET, swift, workflow); - expect(before.unknown).toEqual([]); - - const after = buildReport( + test('a renamed test in the nightly `-skip-testing:` list is reported too', () => { + // Without this the typo is invisible: the nightly would simply stop skipping, run + // testCommand, and hang until timeout-minutes with no clue in the log. + const nightly = fs.readFileSync(path.join(repoRoot, NIGHTLY_WORKFLOW_FILE), 'utf8'); + const typo = `${TARGET}/RunnerTests/testCommandd`; + const report = buildReport( TARGET, - source('extension RunnerTests {\n func testKept() {}\n}\n'), - workflow, + realSources(), + realWorkflows({ + [NIGHTLY_WORKFLOW_FILE]: nightly.replace(`${TARGET}/RunnerTests/testCommand`, typo), + }), ); - expect(after.unknown.map((entry) => entry.identifier)).toEqual([ - `${TARGET}/RunnerTests/testGone`, + + expect(report.unknown.map((entry) => [entry.flag, entry.identifier])).toEqual([ + ['skip-testing', typo], ]); + expect(reportFailures(report).join('\n')).toContain('testCommand'); + }); + + test('a deleted test is reported even though the surviving list still passes', () => { + const workflows = [ + { + workflow: PR_WORKFLOW_FILE, + text: [ + ` -only-testing:${TARGET}/RunnerTests/testKept \\`, + ` -only-testing:${TARGET}/RunnerTests/testGone`, + ].join('\n'), + }, + { workflow: NIGHTLY_WORKFLOW_FILE, text: `-skip-testing:${TARGET}/RunnerTests/testKept` }, + ]; + const kept = 'extension RunnerTests {\n func testKept() {}\n}\n'; + + expect( + buildReport( + TARGET, + source(`${kept}extension RunnerTests {\n func testGone() {}\n}\n`), + workflows, + ).unknown, + ).toEqual([]); + expect( + buildReport(TARGET, source(kept), workflows).unknown.map((entry) => entry.identifier), + ).toEqual([`${TARGET}/RunnerTests/testGone`]); }); }); @@ -141,48 +193,124 @@ describe('the declaration scan', () => { `${TARGET}/RunnerTests/testWithNestedHelper`, ]); }); + + test('reads a file whose name does not start with RunnerTests', () => { + // RunnerTapPointPolicy.swift is the real instance: the synchronized-root-group project + // compiles every .swift in the directory, so file naming carries no membership meaning. + expect( + parseDeclaredTests(TARGET, [ + { + file: 'RunnerTapPointPolicy.swift', + text: 'extension RunnerTests {\n func testGolden() {}\n}\n', + }, + ]), + ).toEqual([`${TARGET}/RunnerTests/testGolden`]); + }); }); describe('the workflow scan', () => { - test('reads every entry on the multi-line xcodebuild invocation', () => { + test('reads both flags on the multi-line xcodebuild invocation', () => { expect( - parseSelectedTests( + parseFlaggedTests( + PR_WORKFLOW_FILE, [ ' xcodebuild test-without-building \\', ' -xctestrun "$XCTESTRUN_PATH" \\', ` -only-testing:${TARGET}/RunnerTests/testOne \\`, - ` -only-testing:${TARGET}/RunnerTests/testTwo`, + ` -skip-testing:${TARGET}/RunnerTests/testTwo`, + ].join('\n'), + ), + ).toEqual([ + { + workflow: PR_WORKFLOW_FILE, + flag: 'only-testing', + identifier: `${TARGET}/RunnerTests/testOne`, + line: 3, + }, + { + workflow: PR_WORKFLOW_FILE, + flag: 'skip-testing', + identifier: `${TARGET}/RunnerTests/testTwo`, + line: 4, + }, + ]); + }); + + test('ignores comments, which is where these workflows discuss their own flags', () => { + // A real defect in this check's first draft: both workflows explain `-only-testing:` + // and `-skip-testing:` in comments, and the scan counted the prose as configuration — + // inflating the reported PR selection and inventing a skip the nightly never makes. + expect( + parseFlaggedTests( + NIGHTLY_WORKFLOW_FILE, + [ + ` # A typo re-arms the hang: -skip-testing:${TARGET}/RunnerTests/testProse`, + ` -skip-testing:${TARGET}/RunnerTests/testReal`, ].join('\n'), ), ).toEqual([ - { identifier: `${TARGET}/RunnerTests/testOne`, line: 3 }, - { identifier: `${TARGET}/RunnerTests/testTwo`, line: 4 }, + { + workflow: NIGHTLY_WORKFLOW_FILE, + flag: 'skip-testing', + identifier: `${TARGET}/RunnerTests/testReal`, + line: 2, + }, ]); }); + test('a bare flag mention with no identifier after it is not a selection', () => { + expect(parseFlaggedTests(PR_WORKFLOW_FILE, 'run: echo "-only-testing: is a flag"')).toEqual([]); + }); + test('leaves another target alone rather than guessing about sources it cannot see', () => { const report = buildReport( TARGET, source('extension RunnerTests {\n func testOne() {}\n}\n'), - `-only-testing:SomeOtherTarget/OtherTests/testUnknown\n-only-testing:${TARGET}/RunnerTests/testOne`, + [ + { + workflow: PR_WORKFLOW_FILE, + text: `-only-testing:SomeOtherTarget/OtherTests/testUnknown\n-only-testing:${TARGET}/RunnerTests/testOne`, + }, + ], ); expect(report.unknown).toEqual([]); - expect(report.selected).toHaveLength(2); + expect(report.flagged).toHaveLength(2); }); }); describe('the blind-parse guards', () => { + const oneTest = () => source('extension RunnerTests {\n func testOne() {}\n}\n'); + const oneFlag = (workflow: string) => [ + { workflow, text: `-only-testing:${TARGET}/RunnerTests/testOne` }, + ]; + + test('a guarded workflow that no longer exists fails instead of leaving a stale claim', () => { + const report = buildReport(TARGET, oneTest(), [ + ...oneFlag(PR_WORKFLOW_FILE), + { workflow: NIGHTLY_WORKFLOW_FILE, text: null }, + ]); + expect(reportFailures(report).join('\n')).toContain(NIGHTLY_WORKFLOW_FILE); + }); + test('an empty declaration scan fails instead of reporting a healthy list', () => { - const report = buildReport(TARGET, source('// nothing here\n'), '-only-testing:a/b/c'); + const report = buildReport(TARGET, source('// nothing here\n'), oneFlag(PR_WORKFLOW_FILE)); expect(reportFailures(report).join('\n')).toContain('declaration scan is broken'); }); test('an empty workflow scan fails instead of reporting a healthy list', () => { - const report = buildReport( - TARGET, - source('extension RunnerTests {\n func testOne() {}\n}\n'), - '', - ); + const report = buildReport(TARGET, oneTest(), [{ workflow: PR_WORKFLOW_FILE, text: '' }]); expect(reportFailures(report).join('\n')).toContain('stopped filtering'); }); }); + +describe('the summary line', () => { + test('reports the partition a reader needs to see the PR lane shrinking', () => { + const report = loadReport(repoRoot); + const { declared, pr, skipped, nightlyOnly } = counts(report); + const summary = formatSummary(report); + expect(summary).toContain(`${declared} declared`); + expect(summary).toContain(`${pr} selected on every PR`); + expect(summary).toContain(`${skipped} skipped by the nightly`); + expect(summary).toContain(`${nightlyOnly} reached only by the nightly`); + }); +}); diff --git a/scripts/check-affected/model.test.ts b/scripts/check-affected/model.test.ts index f358961ce..b753bea55 100644 --- a/scripts/check-affected/model.test.ts +++ b/scripts/check-affected/model.test.ts @@ -82,6 +82,24 @@ test('Swift runner change selects both XCUITest platform builds', () => { assert.ok(ids(['src/platforms/apple/core/runner/Support.swift']).includes('swift-runner-ios')); }); +test('a runner XCTest source also selects the PR test-list check', () => { + // Distinct from the rule above, which owns Swift *anywhere*: renaming a method under + // AgentDeviceRunnerUITests/ silently shrinks ios.yml's hand-written `-only-testing:` list + // (#1781 A7), and the platform builds cannot see that — they compile fine either way. + assert.deepEqual( + ids(['apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Alert.swift']), + ['swift-runner-ios', 'swift-runner-macos', 'xctest-selection'], + ); + // The bug the file filter used to have: membership is the directory, not the name. + assert.ok( + ids([ + 'apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTapPointPolicy.swift', + ]).includes('xctest-selection'), + ); + // Swift elsewhere in the runner still selects only the builds. + assert.ok(!ids(['apple/runner/Sources/Runner/Main.swift']).includes('xctest-selection')); +}); + test('Android helper change selects the android-helpers build', () => { assert.deepEqual(ids(['android/snapshot-helper/src/Main.kt']), ['android-helpers']); assert.deepEqual(ids(['android/ime-helper/AndroidManifest.xml']), ['android-helpers']); diff --git a/scripts/check-xctest-selection.ts b/scripts/check-xctest-selection.ts index d850725a1..3b7ddf8d4 100644 --- a/scripts/check-xctest-selection.ts +++ b/scripts/check-xctest-selection.ts @@ -1,21 +1,23 @@ -// `pnpm check:xctest-selection` — hold the hand-written `-only-testing:` list in -// .github/workflows/ios.yml to the tests that actually exist (#1781 A7). +// `pnpm check:xctest-selection` — hold the hand-written `-only-testing:` and +// `-skip-testing:` lists in the iOS workflows to the tests that actually exist (#1781 A7). // -// The PR lane runs a subset of the runner XCTest suite by naming each method on the -// xcodebuild command line. `xcodebuild test-without-building` treats an -// `-only-testing:` identifier that matches nothing as an empty selection rather than an -// error, so a renamed or deleted test does not fail the lane — it silently stops being -// tested, and the lane stays green with fewer tests than the list claims. That failure -// mode is invisible in logs unless someone counts, which is what this check does. +// `xcodebuild` treats a test identifier that matches nothing as an empty set rather than an +// error, in BOTH directions, and each direction fails silently in its own way: // -// Deliberately one-directional: a test that exists but is NOT in the PR list is fine, it -// runs in the nightly full-suite lane (.github/workflows/xctest-nightly.yml). Only the -// reverse — a listed name no source declares — is a defect. +// - `-only-testing:` (ios.yml, 37 hand-written entries) — a renamed or deleted test stops +// running with no signal. The lane stays green with fewer tests than the list claims. +// - `-skip-testing:` (xctest-nightly.yml) — the nightly skips `RunnerTests/testCommand`, +// which is not a test at all: it is the runner's server entry point (RunnerTests.swift), +// compiled unconditionally, and it starts an NWListener and waits 24 hours. A typo in +// that entry re-arms a full-timeout hang, and a hang is the most expensive way a lane +// can fail. // -// The declaration scan is source-level, so a method compiled out by an `#if os(...)` -// guard still counts as declared. That is the intended precision: this check guards the -// list against renames and deletions, not against platform availability, which the -// nightly full-suite run observes directly. +// Deliberately one-directional about coverage: a test in no `-only-testing:` list is fine, +// the nightly runs it. Only an identifier naming nothing is a defect. +// +// The declaration scan is source-level, so a method compiled out by `#if` still counts as +// declared. That is the intended precision — this check guards the lists against renames and +// deletions, not against platform availability, which the nightly observes directly. import fs from 'node:fs'; import path from 'node:path'; @@ -26,13 +28,23 @@ const repoRoot = path.resolve(import.meta.dirname, '..'); /** The XCTest target directory; its basename is the target name the identifiers use. */ export const RUNNER_TESTS_DIR = 'apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests'; -/** The workflow whose `-only-testing:` list this check guards. */ +/** The PR lane, whose `-only-testing:` list decides what every pull request runs. */ export const PR_WORKFLOW_FILE = '.github/workflows/ios.yml'; -/** The workflow that runs everything the PR list leaves out. */ -const NIGHTLY_WORKFLOW_FILE = '.github/workflows/xctest-nightly.yml'; +/** The nightly lane, whose `-skip-testing:` list decides what the full suite leaves out. */ +export const NIGHTLY_WORKFLOW_FILE = '.github/workflows/xctest-nightly.yml'; + +/** + * Every workflow whose test identifiers this check owns. Both are read, so a workflow that + * is renamed or deleted fails here rather than leaving a stale claim in the output. + */ +export const GUARDED_WORKFLOWS: readonly string[] = [PR_WORKFLOW_FILE, NIGHTLY_WORKFLOW_FILE]; -const RUNNER_TESTS_SOURCE = /^RunnerTests.*\.swift$/; +// Every .swift file in the target directory is a member: the Xcode project uses a +// PBXFileSystemSynchronizedRootGroup, so membership is the directory, not a file list. A +// `RunnerTests*` name filter would miss RunnerTapPointPolicy.swift, which declares a real +// addressable test inside `extension RunnerTests`. +const SWIFT_SOURCE = /\.swift$/; // One ordered pass over the source. A column-0 type declaration moves the enclosing type; // a `func test…` indented exactly one level binds to it. Position carries the meaning @@ -44,27 +56,46 @@ const RUNNER_TESTS_SOURCE = /^RunnerTests.*\.swift$/; const DECLARATION = /^(?:[\w@]+[ \t]+)*(?:class|extension|struct|enum|actor|protocol)[ \t]+([A-Za-z_]\w*)|^ {2}(?:[\w@]+[ \t]+)*func[ \t]+(test\w*)[ \t]*\(/gm; -const ONLY_TESTING = /-only-testing:(\S+)/; +// Two guards against reading prose as configuration, both learned the hard way: these +// workflows discuss their own flags in comments, and this check's first draft counted the +// comments. A line whose first non-space character is `#` is a comment in YAML and in the +// `run:` shell alike, so it can never be a flag xcodebuild sees; and the identifier must +// have the `Target/Class/method` shape, so a prose mention with no identifier after the +// colon matches nothing. A typo'd identifier is still identifier-shaped, so both guards +// narrow what counts as a flag without narrowing what counts as a defect. +const YAML_COMMENT = /^\s*#/; +const TEST_FLAG = /-(only|skip)-testing:([A-Za-z_][\w.+-]*(?:\/[A-Za-z_]\w*){1,2})/; export type SwiftSource = { readonly file: string; readonly text: string }; -/** One `-only-testing:` identifier as written, with the line that carries it. */ -export type SelectedTest = { readonly identifier: string; readonly line: number }; +export type TestFlag = 'only-testing' | 'skip-testing'; + +/** One `-only-testing:`/`-skip-testing:` identifier as written, and where it was written. */ +export type FlaggedTest = { + readonly workflow: string; + readonly flag: TestFlag; + readonly identifier: string; + readonly line: number; +}; export type SelectionReport = { readonly target: string; /** Every `Target/Class/method` the Swift sources declare, sorted. */ readonly declared: readonly string[]; - /** Every `-only-testing:` identifier in the PR workflow, in file order. */ - readonly selected: readonly SelectedTest[]; - /** Selected identifiers naming no declared method — the failure. */ - readonly unknown: readonly SelectedTest[]; + /** Every flagged identifier across the guarded workflows, in file order. */ + readonly flagged: readonly FlaggedTest[]; + /** Guarded workflows that do not exist — a claim this check can no longer make. */ + readonly missingWorkflows: readonly string[]; + /** Flagged identifiers naming no declared method — the failure. */ + readonly unknown: readonly FlaggedTest[]; }; +export type WorkflowSource = { readonly workflow: string; readonly text: string | null }; + export function readSwiftSources(directory: string): SwiftSource[] { return fs .readdirSync(directory) - .filter((entry) => RUNNER_TESTS_SOURCE.test(entry)) + .filter((entry) => SWIFT_SOURCE.test(entry)) .sort() .map((entry) => ({ file: entry, @@ -85,29 +116,41 @@ export function parseDeclaredTests(target: string, sources: readonly SwiftSource return [...declared].sort(); } -/** Every `-only-testing:` identifier the workflow names, with its line number. */ -export function parseSelectedTests(workflowText: string): SelectedTest[] { - return workflowText.split('\n').flatMap((text, index) => { - const identifier = ONLY_TESTING.exec(text)?.[1]; - return identifier ? [{ identifier, line: index + 1 }] : []; +/** Every `-only-testing:`/`-skip-testing:` identifier a workflow names, with its line. */ +export function parseFlaggedTests(workflow: string, text: string): FlaggedTest[] { + return text.split('\n').flatMap((line, index) => { + if (YAML_COMMENT.test(line)) return []; + const match = TEST_FLAG.exec(line); + if (!match) return []; + return [ + { + workflow, + flag: `${match[1]}-testing` as TestFlag, + identifier: match[2] as string, + line: index + 1, + }, + ]; }); } export function buildReport( target: string, sources: readonly SwiftSource[], - workflowText: string, + workflows: readonly WorkflowSource[], ): SelectionReport { const declared = parseDeclaredTests(target, sources); const known = new Set(declared); - const selected = parseSelectedTests(workflowText); + const flagged = workflows.flatMap((entry) => + entry.text === null ? [] : parseFlaggedTests(entry.workflow, entry.text), + ); return { target, declared, - selected, + flagged, + missingWorkflows: workflows.filter((entry) => entry.text === null).map((e) => e.workflow), // Identifiers for another target are left alone: this check owns one target's - // sources and cannot speak for anything else the workflow might select. - unknown: selected.filter( + // sources and cannot speak for anything else a workflow might select. + unknown: flagged.filter( (entry) => entry.identifier.startsWith(`${target}/`) && !known.has(entry.identifier), ), }; @@ -118,43 +161,84 @@ export function loadReport(root: string = repoRoot): SelectionReport { return buildReport( path.basename(directory), readSwiftSources(directory), - fs.readFileSync(path.join(root, PR_WORKFLOW_FILE), 'utf8'), + GUARDED_WORKFLOWS.map((workflow) => { + const file = path.join(root, workflow); + return { workflow, text: fs.existsSync(file) ? fs.readFileSync(file, 'utf8') : null }; + }), ); } -/** The failures, or an empty list. Kept separate from formatting so the test can assert both. */ +/** Attributed per workflow, not just per flag: each lane's number has to be its own. */ +function identifiers(report: SelectionReport, workflow: string, flag: TestFlag): Set { + return new Set( + report.flagged + .filter((entry) => entry.workflow === workflow && entry.flag === flag) + .map((entry) => entry.identifier), + ); +} + +/** What each lane reaches, once the two flags are resolved against the declared set. */ +export function counts(report: SelectionReport): { + declared: number; + pr: number; + skipped: number; + nightlyOnly: number; +} { + const pr = identifiers(report, PR_WORKFLOW_FILE, 'only-testing'); + const skipped = identifiers(report, NIGHTLY_WORKFLOW_FILE, 'skip-testing'); + return { + declared: report.declared.length, + pr: pr.size, + skipped: skipped.size, + nightlyOnly: report.declared.filter((id) => !pr.has(id) && !skipped.has(id)).length, + }; +} + +/** The failures, or an empty list. Kept apart from formatting so the test can assert both. */ export function reportFailures(report: SelectionReport): string[] { // A parser that stops matching would report "0 unknown" forever, which reads exactly // like a healthy list. Both inputs are non-empty by construction, so an empty parse is // the check going blind, not a real state of the tree. + if (report.missingWorkflows.length > 0) { + return [ + `Missing guarded workflow(s): ${report.missingWorkflows.join(', ')}. This check names ` + + 'them in its own output, so a renamed or deleted lane must be reflected in ' + + 'GUARDED_WORKFLOWS rather than leaving a claim nothing backs.', + ]; + } if (report.declared.length === 0) { return [ `Found no test methods in ${RUNNER_TESTS_DIR}. The declaration scan is broken, ` + 'so this check can no longer see a dropped test.', ]; } - if (report.selected.length === 0) { + if (report.flagged.length === 0) { return [ - `Found no \`-only-testing:\` entries in ${PR_WORKFLOW_FILE}. Either the PR lane stopped ` + - 'filtering (drop this check), or the scan is broken and can no longer see a dropped test.', + 'Found no `-only-testing:`/`-skip-testing:` entries in ' + + `${GUARDED_WORKFLOWS.join(', ')}. Either both lanes stopped filtering (drop this ` + + 'check), or the scan is broken and can no longer see a dropped test.', ]; } if (report.unknown.length === 0) return []; return [ - `${PR_WORKFLOW_FILE} selects ${report.unknown.length} XCTest method(s) that no source declares:`, - ...report.unknown.map((entry) => ` - line ${entry.line}: ${entry.identifier}`), - 'xcodebuild runs nothing for an unmatched `-only-testing:` identifier and still exits 0, so a', - `rename or deletion drops the test silently. Update the entry to the current name, or remove`, - `it — ${NIGHTLY_WORKFLOW_FILE} runs the whole suite either way.`, + `${report.unknown.length} XCTest identifier(s) name a method no source declares:`, + ...report.unknown.map( + (entry) => ` - ${entry.workflow}:${entry.line} (-${entry.flag}) ${entry.identifier}`, + ), + 'xcodebuild matches nothing and still exits 0 for an unknown identifier, in both', + 'directions: an unknown `-only-testing:` drops a test from the PR lane silently, and an', + 'unknown `-skip-testing:` re-admits whatever the nightly meant to leave out — including', + `${report.target}/RunnerTests/testCommand, the runner's 24-hour server entry point.`, + 'Update the entry to the current name, or remove it.', ]; } export function formatSummary(report: SelectionReport): string { - const remaining = report.declared.length - report.selected.length; + const { declared, pr, skipped, nightlyOnly } = counts(report); return ( - `xctest selection: ${report.selected.length} of ${report.declared.length} ${report.target} ` + - `methods run on every PR (${PR_WORKFLOW_FILE}); the other ${remaining} run in ` + - `${NIGHTLY_WORKFLOW_FILE}.\n` + `xctest selection: ${declared} declared ${report.target} methods — ${pr} selected on ` + + `every PR (${PR_WORKFLOW_FILE}), ${skipped} skipped by the nightly ` + + `(${NIGHTLY_WORKFLOW_FILE}), ${nightlyOnly} reached only by the nightly.\n` ); } diff --git a/scripts/xctest-run-summary.ts b/scripts/xctest-run-summary.ts new file mode 100644 index 000000000..eb7f95a07 --- /dev/null +++ b/scripts/xctest-run-summary.ts @@ -0,0 +1,100 @@ +// Renders the nightly XCTest lane's job summary, and asserts the run executed tests. +// +// Two jobs in one place because they read the same file. `xcodebuild` exits 0 when a test +// selection matches nothing, so "green" and "ran no tests" produce the same log tail: a +// build variant missing the unit-test compile flag, an empty test plan, a renamed target, +// or a `-skip-testing:` entry that swallowed the suite would all read as a healthy night. +// The executed-count assertion is what tells those apart. +// +// A script rather than `node -e` in the workflow: this is quoting-sensitive string building +// inside YAML inside shell, and it has a cap to enforce — the job summary is limited to +// 1 MiB, and the night this lane matters most is the night the failure list is longest. +// +// Reads `xcrun xcresulttool get test-results summary` JSON (Xcode 16+ shape). + +import fs from 'node:fs'; +import { pathToFileURL } from 'node:url'; + +/** Failure entries beyond this are counted, not listed. */ +export const MAX_LISTED_FAILURES = 60; + +/** Each listed failure's message is truncated to this, so one stack cannot dominate. */ +export const MAX_FAILURE_TEXT = 300; + +type Failure = { + testName?: string; + testIdentifierString?: string; + failureText?: string; +}; + +export type ResultSummary = { + result?: string; + totalTestCount?: number; + passedTests?: number; + failedTests?: number; + skippedTests?: number; + expectedFailures?: number; + startTime?: number; + finishTime?: number; + testFailures?: Failure[]; +}; + +function duration(summary: ResultSummary): string { + const { startTime, finishTime } = summary; + if (typeof startTime !== 'number' || typeof finishTime !== 'number') return 'unknown'; + return `${Math.max(0, Math.round(finishTime - startTime))}s`; +} + +function failureLine(failure: Failure): string { + const name = failure.testName ?? failure.testIdentifierString ?? '(unnamed)'; + const text = String(failure.failureText ?? '') + .replace(/\s+/g, ' ') + .slice(0, MAX_FAILURE_TEXT); + return `- \`${name}\`${text ? ` — ${text}` : ''}`; +} + +export function renderSummary(summary: ResultSummary): string { + const failures = Array.isArray(summary.testFailures) ? summary.testFailures : []; + const shown = failures.slice(0, MAX_LISTED_FAILURES); + const lines = [ + '### iOS runner full XCTest suite', + '', + `- result: **${summary.result ?? 'unknown'}**`, + `- executed: **${summary.totalTestCount ?? 0}** (passed ${summary.passedTests ?? 0}, ` + + `failed ${summary.failedTests ?? 0}, skipped ${summary.skippedTests ?? 0}, ` + + `expected failures ${summary.expectedFailures ?? 0})`, + `- duration: ${duration(summary)}`, + ]; + if (shown.length > 0) { + lines.push('', '#### Failures', ...shown.map(failureLine)); + if (failures.length > shown.length) { + lines.push(`- …and ${failures.length - shown.length} more; see the uploaded result bundle.`); + } + } + return `${lines.join('\n')}\n`; +} + +/** Null when the run is credible, or the operator-facing reason it is not. */ +export function livenessFailure(summary: ResultSummary): string | null { + if ((summary.totalTestCount ?? 0) >= 1) return null; + return [ + 'The full-suite lane executed no tests, which xcodebuild reports as success.', + 'Check AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS (the -D flag that compiles the tests in),', + 'the xctestrun test plan, the target name, and the -skip-testing entry.', + ].join('\n'); +} + +function main(): number { + const summaryPath = process.env.RESULT_SUMMARY_PATH; + if (!summaryPath) throw new Error('RESULT_SUMMARY_PATH is not set.'); + const summary = JSON.parse(fs.readFileSync(summaryPath, 'utf8')) as ResultSummary; + const stepSummary = process.env.GITHUB_STEP_SUMMARY; + if (stepSummary) fs.appendFileSync(stepSummary, renderSummary(summary)); + process.stdout.write(`Executed ${summary.totalTestCount ?? 0} test(s).\n`); + const failure = livenessFailure(summary); + if (!failure) return 0; + process.stderr.write(`${failure}\n`); + return 1; +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) process.exit(main()); diff --git a/vitest.config.ts b/vitest.config.ts index cfe7b7cde..d3962688e 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -80,6 +80,9 @@ export default defineConfig({ // Parses ios.yml and the runner's Swift sources: no Xcode, no simulator, and // the check it guards is what keeps the PR lane's `-only-testing:` list honest. 'scripts/__tests__/xctest-selection.test.ts', + // The nightly XCTest lane's reporter/liveness check, which otherwise only ever + // executes on a macOS runner at 04:30. + 'scripts/__tests__/xctest-run-summary.test.ts', // The Fallow fixture policy is executable configuration: unused exports are exempt, // but fixture modules remain visible to the other analysis families. 'scripts/__tests__/fallow-fixture-policy.test.ts', From bfdda1979f3b3552ece1691ba1c45f23b5bed09e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 19:33:09 +0200 Subject: [PATCH 3/5] docs(ci): restate the nightly lane cost and timeout honestly --- .github/workflows/xctest-nightly.yml | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/workflows/xctest-nightly.yml b/.github/workflows/xctest-nightly.yml index 332202ac1..a189c1e47 100644 --- a/.github/workflows/xctest-nightly.yml +++ b/.github/workflows/xctest-nightly.yml @@ -16,9 +16,11 @@ name: XCTest Nightly # - Evidence: the list has never been re-derived since it was written by hand; the tests it # omits have been dark for their whole life, so their current state is unmeasured. The # first red night is the evidence this lane exists to produce. -# - Cost: one macOS job per night. The Apple runner build is shared with ios.yml's cache -# (identical inputs and build variant), so a night that follows a green ios.yml run pays -# only the boot + full-suite time on top of a cache restore. Adds nothing to any PR. +# - Cost: this is a public repository, so nothing here is billed. The real cost is one macOS +# concurrency slot per night. The Apple runner build shares ios.yml's cache (identical +# inputs and build variant), but scheduled runs only see DEFAULT-BRANCH caches, so the hit +# depends on ios.yml's `push: main` run having populated the cache at the same source +# hash — on a miss this lane also pays a full runner build. Adds nothing to any PR. # - Kill criterion: when A7's classification lands — pure Swift decision tests moved to a # fast unit target, stale tests deleted, the rest kept on the simulator — this lane goes # if the surviving set is small enough to run on every PR. @@ -49,8 +51,10 @@ jobs: full-suite: name: iOS Runner Full XCTest Suite runs-on: macos-26 - # Generous on purpose: the PR lane's 37 tests are a few minutes inside an 80-minute job - # that also runs replays and E2E, and this lane's total is unmeasured until it has run. + # Provisional, and deliberately generous: this lane's wall time is unmeasured until it has + # run once, and two of the tests it newly reaches carry intentional 15s/6s waits. Tighten + # to roughly twice the first measured duration — a ceiling this loose turns a wedged run + # into two hours of held slot before anyone hears about it. timeout-minutes: 120 env: # Must match ios.yml. The runtime version rides in the derived-data cache key, and From 8e54e29e1b5822eed841f1cf05590b7dd0baa551 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 10:04:39 +0200 Subject: [PATCH 4/5] docs(ci): stop quoting XCTest counts that drift between commits --- .github/workflows/ci.yml | 2 +- .github/workflows/xctest-nightly.yml | 29 +++++++++++++++++----------- docs/agents/testing.md | 2 +- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 82c4a31b6..f9ab488bb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -178,7 +178,7 @@ jobs: with: { gate: gate-manifest } # Same family as the manifest above — a CI selection that has stopped selecting what - # it claims. ios.yml runs 37 of the 154 runner XCTest methods through a hand-written + # it claims. ios.yml runs a hand-written subset of the runner XCTest methods through an # `-only-testing:` list, and xcodebuild treats an identifier that matches nothing as # an empty selection rather than an error, so a rename drops a test with no signal — # in both directions, since a typo in xctest-nightly.yml's `-skip-testing:` entry diff --git a/.github/workflows/xctest-nightly.yml b/.github/workflows/xctest-nightly.yml index a189c1e47..8a8fba328 100644 --- a/.github/workflows/xctest-nightly.yml +++ b/.github/workflows/xctest-nightly.yml @@ -1,18 +1,25 @@ name: XCTest Nightly -# The full iOS runner XCTest suite (#1781 A7). The target declares 154 addressable methods; -# the PR lane (ios.yml) names 37 of them in a hand-written `-only-testing:` list, so 117 ran -# nowhere at all. This lane drops that filter and runs the test plan whole, skipping only +# The full iOS runner XCTest suite (#1781 A7). The PR lane (ios.yml) names a hand-written +# subset of the target's methods in an `-only-testing:` list; everything outside that list ran +# nowhere at all. This lane drops the filter and runs the test plan whole, skipping only # `testCommand` — which is the runner's server entry point rather than a test (see the step -# below) — for 116 methods that nothing else executes. +# below). +# +# No count is quoted here on purpose. `pnpm check:xctest-selection` prints the live split +# (declared / PR-selected / skipped / nightly-only) and is the only place those numbers are +# derived rather than remembered. A hand-copied figure in a comment is the same class of +# defect this lane exists to close: between this file being written and first review, the +# suite gained three methods and the PR list gained five entries, so every number originally +# written here was already wrong. # # Under the lane rule in #1781: # -# - Catches: regressions in the 116 runner-semantics tests the PR list leaves out — -# selector matching, snapshot traversal, command journal, sequence execution, tv remote, -# keyboard, scroll/gesture policy. Nothing else executes them. Two of the 154 -# (`…OnTvOS`) are behind `#if os(tvOS)` and so never execute on this lane's destination; -# the honest iOS-reachable figure is 151. +# - Catches: regressions in the runner-semantics tests the PR list leaves out — selector +# matching, snapshot traversal, command journal, sequence execution, tv remote, keyboard, +# scroll/gesture policy. Nothing else executes them. Note the reachable set is smaller +# than the declared set: `testCommand` is skipped, and the `…OnTvOS` tests sit behind +# `#if os(tvOS)` and cannot execute on this lane's iOS Simulator destination. # - Evidence: the list has never been re-derived since it was written by hand; the tests it # omits have been dark for their whole life, so their current state is unmeasured. The # first red night is the evidence this lane exists to produce. @@ -26,8 +33,8 @@ name: XCTest Nightly # if the surviving set is small enough to run on every PR. # # Scheduled + manual only. It is deliberately NOT a PR gate: ios.yml is already over half of -# this repo's CI spend (#1781 A9), and adding 116 tests to it would make that worse for a set -# of tests whose flakiness on a shared runner is unknown. +# this repo's CI spend (#1781 A9), and moving the unrun set onto it would make that worse for +# tests whose flakiness on a shared runner is unknown. # # Note on `concurrency` below: it is keyed on `github.ref`, so a manual dispatch on the same # ref cancels an in-flight scheduled run. That is the intended trade — an operator asking for diff --git a/docs/agents/testing.md b/docs/agents/testing.md index ef1d19d7b..1344faef8 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -37,7 +37,7 @@ The mapping it encodes, for when you need to run a gate directly or reason about | Platform/device response — anything emitting `platform`/`appleOs` on the wire, or shaping a daemon response | `pnpm test:integration:provider` **and** `pnpm test:coverage` | | Cross-platform behavior | `pnpm test:integration` | | Apple runner / Swift | Build the changed target with `pnpm build:xcuitest:`; use `pnpm build:xcuitest` only for shared iOS/macOS changes | -| Runner XCTest methods (`apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/**`) | `pnpm check:xctest-selection`. `ios.yml` runs 37 of the 154 methods through a hand-written `-only-testing:` list and `xctest-nightly.yml` excludes one via `-skip-testing:`; `xcodebuild` treats an identifier matching nothing as an empty selection rather than an error, in both directions — a rename drops a test from the PR lane silently, or re-admits `RunnerTests/testCommand`, the runner's 24-hour server entry point, into the nightly and hangs it. The check fails only on a listed name no source declares; a test in no list is expected, because the nightly runs the suite whole | +| Runner XCTest methods (`apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/**`) | `pnpm check:xctest-selection`, which prints the live declared/PR-selected/skipped/nightly-only split — the counts move often enough that quoting one here would rot. `ios.yml` runs a hand-written subset through an `-only-testing:` list and `xctest-nightly.yml` excludes one method via `-skip-testing:`; `xcodebuild` treats an identifier matching nothing as an empty selection rather than an error, in both directions — a rename drops a test from the PR lane silently, or re-admits `RunnerTests/testCommand`, the runner's 24-hour server entry point, into the nightly and hangs it. The check fails only on a listed name no source declares; a test in no list is expected, because the nightly runs the suite whole | | CLI help/guidance (`src/cli/parser/cli-help.ts`, `src/cli-schema/`) | `pnpm exec vitest run src/cli/parser/__tests__ src/cli-schema/command-schema-guards.test.ts scripts/__tests__` — the `scripts/__tests__` gates enforce help-topic benchmark coverage and pin the bench's quoted CLI samples to the real renderers | | Help benchmark cases (`scripts/help-conformance-*.mjs`) | `pnpm exec vitest run scripts/__tests__` (deterministic gates); model-backed: `pnpm bench:help-conformance` (paid LLM calls, local only) | | `.ad` grammar (`src/replay/script.ts`, gesture arity, replay vars) | `pnpm exec vitest run --project unit-core test/replay-compat` — the frozen replay-compat corpus asserts which released script surfaces still parse; a flipped verdict is edited in `test/replay-compat/manifest.ts`, never in the script. Adding or re-pinning a corpus entry also runs `pnpm check:replay-compat`, which re-derives each entry from its release tag in git history | From ec11c8f113248a4640249b01afd9893ebb183432 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 10:32:42 +0200 Subject: [PATCH 5/5] ci(ios): tighten the nightly timeout to the measured suite duration --- .github/workflows/xctest-nightly.yml | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/.github/workflows/xctest-nightly.yml b/.github/workflows/xctest-nightly.yml index 8a8fba328..52b74da71 100644 --- a/.github/workflows/xctest-nightly.yml +++ b/.github/workflows/xctest-nightly.yml @@ -58,11 +58,14 @@ jobs: full-suite: name: iOS Runner Full XCTest Suite runs-on: macos-26 - # Provisional, and deliberately generous: this lane's wall time is unmeasured until it has - # run once, and two of the tests it newly reaches carry intentional 15s/6s waits. Tighten - # to roughly twice the first measured duration — a ceiling this loose turns a wedged run - # into two hours of held slot before anyone hears about it. - timeout-minutes: 120 + # Measured, not guessed. A full local run of this exact command on matching toolchain + # (Xcode 26.2, iOS 26.2 Simulator) executed 154 tests in 120s wall — the suite itself is + # ~2 minutes, not the bulk of the job. What dominates the budget is the runner build on a + # derived-data cache MISS, plus checkout and simulator boot. ios.yml carries 80 minutes + # while doing strictly more than this lane (same build and boot, plus replays, prepare, + # and the fixture E2E), so half of that is generous headroom for a cache-miss night and + # still caps a wedged run at 45 minutes instead of two hours of held macOS slot. + timeout-minutes: 45 env: # Must match ios.yml. The runtime version rides in the derived-data cache key, and # AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS is BOTH an input to the cache's build-variant