fix(ci): skip the runner server entry point in the nightly and validate both test flags

Author: thymikee

.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 }

.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()

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:<platform>`; 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 |

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()`');
+  });
+});