diff --git a/.github/workflows/ci-macos.yml b/.github/workflows/ci-macos.yml index 32e3279d75ab..61609a40b250 100644 --- a/.github/workflows/ci-macos.yml +++ b/.github/workflows/ci-macos.yml @@ -28,6 +28,20 @@ on: required: false default: "" type: string + # Space-separated cmuxTests/ suites when `unit_suite` came from a diff + # that only edits those suites. `app-host unit tests` then runs just + # them on one worker instead of every suite on seven. Empty runs all. + unit_selectors: + required: false + default: "" + type: string + # `|`-delimited names of the strict steps below that own a suite in + # unit_selectors. A changed-suites run runs those steps as they are, + # since each gives its suites an app host and settings of their own. + unit_strict_steps: + required: false + default: "" + type: string product_artifacts: required: false default: "layered" @@ -967,7 +981,7 @@ jobs: # transitive skip otherwise marks every macOS job skipped even when # linux-preflight itself succeeds. Require the direct needs explicitly. if: ${{ !cancelled() && needs.macos-compile-admission.result == 'success' && inputs.macos == 'true' && (inputs.full_suite == 'true' || inputs.unit_suite == 'true') }} - name: app-host unit tests (${{ matrix.shard }}/7) + name: ${{ inputs.unit_selectors != '' && 'app-host unit tests (changed suites)' || format('app-host unit tests ({0}/7)', matrix.shard) }} # App-host XCTest needs a runner that can broker testmanagerd control # sessions, so route through the shared MACOS_RUNNER_15 var like the other # macOS jobs. The fallback is what fork pull requests get, because @@ -980,7 +994,9 @@ jobs: # only needs a verdict, and the first failure already is one. fail-fast: ${{ github.event_name == 'merge_group' }} matrix: - shard: [1, 2, 3, 4, 5, 6, 7] + # Shard 8 matches none of the numbered shards below that own a strict + # step, so a changed-suites run executes only its own batch. + shard: ${{ fromJSON(inputs.unit_selectors != '' && '[8]' || '[1, 2, 3, 4, 5, 6, 7]') }} env: CMUX_NODE_PRODUCT_CACHE_ROOT: ${{ vars.CMUX_NODE_PRODUCT_CACHE_ROOT }} CMUX_NODE_PRODUCT_CACHE_MAX_BYTES: ${{ vars.CMUX_NODE_PRODUCT_CACHE_MAX_BYTES }} @@ -991,6 +1007,7 @@ jobs: # closed if a setup step or environment handoff loses either redirect. CMUX_CI_APP_HOST_ISOLATION_REQUIRED: "1" CMUX_APP_HOST_SHARD: ${{ matrix.shard }} + CMUX_APP_HOST_UNIT_SELECTORS: ${{ inputs.unit_selectors }} CMUX_APP_HOST_CAPTURE_XCRESULTS: "1" CMUX_CI_XCODE_APP: ${{ github.event_name == 'pull_request' && (vars.CMUX_CI_XCODE_APP_PR || vars.CMUX_CI_XCODE_APP_MACOS_15) || vars.CMUX_CI_XCODE_APP_MACOS_15 }} CMUX_CI_REQUIRED_MACOS_SDK_MAJOR: "26" @@ -1241,7 +1258,7 @@ jobs: echo "CMUX_APP_HOST_TEST_INVENTORY=$inventory_json" >> "$GITHUB_ENV" - name: Upload built app-host test inventory - if: ${{ matrix.shard == 1 }} + if: ${{ matrix.shard == 1 || inputs.unit_selectors != '' }} uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: cmux-app-host-test-inventory-${{ github.run_id }}-${{ github.run_attempt }} @@ -1266,7 +1283,7 @@ jobs: fi - name: Run My Devices regressions - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) || contains(inputs.unit_strict_steps, '|Run My Devices regressions|')) }} env: CMUX_APP_HOST_XCODEBUILD_ATTEMPTS: "1" run: | @@ -1315,7 +1332,7 @@ jobs: test-without-building - name: Run Pi Feed ownership regressions - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) || contains(inputs.unit_strict_steps, '|Run Pi Feed ownership regressions|')) }} run: | # Swift Testing assertion failures are tolerated in the full sharded # app-host suite. Keep Feed ingestion and ownership on a non-tolerant @@ -1339,7 +1356,7 @@ jobs: - name: Run remote tmux mirror detach and placement regressions id: remote-tmux-mirror - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) || contains(inputs.unit_strict_steps, '|Run remote tmux mirror detach and placement regressions|')) }} run: | # Closing a mirrored workspace must never kill the remote tmux session, # and --new-window must consolidate mirrors moved across source windows. @@ -1444,7 +1461,7 @@ jobs: retention-days: 7 - name: Run focused browser regressions - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) || contains(inputs.unit_strict_steps, '|Run focused browser regressions|')) }} run: | # Focused gates for https://github.com/manaflow-ai/cmux/issues/7380 and #8054. # The tolerant full-suite step can crash-skip late suites (see #5888 @@ -1467,7 +1484,7 @@ jobs: done - name: Run remote tmux mirror active-pane regression - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) || contains(inputs.unit_strict_steps, '|Run remote tmux mirror active-pane regression|')) }} run: | # Focused gates for https://github.com/manaflow-ai/cmux/issues/7372. # The tolerant full-suite step can crash-skip late suites (see #5888 @@ -1483,7 +1500,7 @@ jobs: test-without-building - name: Run remote tmux mirror layout identity regression - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) || contains(inputs.unit_strict_steps, '|Run remote tmux mirror layout identity regression|')) }} run: | # The tolerant full-suite step may accept expected failures or stop # after an app-host crash. Keep #7833's identity suite non-tolerant @@ -1498,7 +1515,7 @@ jobs: test-without-building - name: Run socket ACL reload regression - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) || contains(inputs.unit_strict_steps, '|Run socket ACL reload regression|')) }} run: | # The tolerant full-suite step can accept assertion failures or # crash-skip late suites. Keep #7984's live reload and denial tests @@ -1531,7 +1548,7 @@ jobs: CMUX_CLI_BIN="$CLI_BIN" python3 tests/test_cli_creation_initial_command.py - name: Run settings file-store no-op persistence regression - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) || contains(inputs.unit_strict_steps, '|Run settings file-store no-op persistence regression|')) }} run: | # The broad app-host suite tolerates expected failures and can stop # after an app-host crash. Keep the settings persistence regression @@ -1546,7 +1563,7 @@ jobs: test-without-building - name: Run issue 8373 sidebar convergence regression - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) || contains(inputs.unit_strict_steps, '|Run issue 8373 sidebar convergence regression|')) }} timeout-minutes: 5 run: | # The broad app-host suite tolerates Swift Testing assertion failures. @@ -1572,7 +1589,7 @@ jobs: grep -F "ISSUE_8373_STRESS " "$test_log" - name: Run sidebar workspace-switch layout regression - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) || contains(inputs.unit_strict_steps, '|Run sidebar workspace-switch layout regression|')) }} run: | # The tolerant full-suite step can accept Swift Testing assertion # failures when XCTest reports "0 unexpected". Keep #9612's real @@ -1588,7 +1605,7 @@ jobs: test-without-building - name: Run terminal portal visibility regressions - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) || contains(inputs.unit_strict_steps, '|Run terminal portal visibility regressions|')) }} run: | # These tests mutate the process-wide terminal portal registry and # include Swift Testing assertions that the tolerant broad shard can @@ -1630,7 +1647,7 @@ jobs: node-version: "20" - name: Run Ghostty numeric locale tests - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD) || contains(inputs.unit_strict_steps, '|Run Ghostty numeric locale tests|')) }} run: | # LC_NUMERIC is process-global. Keep these tests in their own app-host # invocation and disable Xcode's parallel test execution for it. @@ -1645,7 +1662,7 @@ jobs: test-without-building - name: Run global search shortcut regressions - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_GLOBAL_SEARCH_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_GLOBAL_SEARCH_SHARD) || contains(inputs.unit_strict_steps, '|Run global search shortcut regressions|')) }} timeout-minutes: 40 run: | # This serialized container owns global shortcut settings. Keep its @@ -1673,7 +1690,7 @@ jobs: "$test_log" cmuxTests/GlobalSearchShortcutBehaviorTests - name: Run Cloud machine ordering acceptance - if: ${{ !cancelled() && steps.prepare-app-host-home.outcome == 'success' && matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) }} + if: ${{ !cancelled() && steps.prepare-app-host-home.outcome == 'success' && (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) || contains(inputs.unit_strict_steps, '|Run Cloud machine ordering acceptance|')) }} timeout-minutes: 10 run: | # Separate invocations prove each selector executed tests; a healthy @@ -1705,6 +1722,11 @@ jobs: LOGICAL_BATCHES_PER_WORKER=2 LOGICAL_SHARD_TOTAL=$((PHYSICAL_SHARD_TOTAL * LOGICAL_BATCHES_PER_WORKER)) LOGICAL_SHARDS=("$PHYSICAL_SHARD" "$((PHYSICAL_SHARD + PHYSICAL_SHARD_TOTAL))") + if [ -n "${CMUX_APP_HOST_UNIT_SELECTORS:-}" ]; then + # A changed-suites run: one batch holding exactly those suites. + LOGICAL_SHARD_TOTAL=1 + LOGICAL_SHARDS=(1) + fi # Each worker runs two balanced batches sequentially. Every invocation # writes to a regular file, and this script mirrors that file into the @@ -1748,16 +1770,34 @@ jobs: for reservation in ${CMUX_APP_HOST_RESERVED_WALL_SECONDS:-}; do reserve_args+=(--reserve "$reservation") done - python3 scripts/ci/cmux_unit_test_shard.py \ - --shard-index "$logical_shard" \ - --shard-total "$LOGICAL_SHARD_TOTAL" \ - --physical-shard-total "$PHYSICAL_SHARD_TOTAL" \ - ${reserve_args[@]+"${reserve_args[@]}"} \ - --output "$shard_args" || { - local shard_status=$? - echo "Failed to generate app-host unit-test batch ${logical_shard}/${LOGICAL_SHARD_TOTAL}" - return "$shard_status" - } + if [ -n "${CMUX_APP_HOST_UNIT_SELECTORS:-}" ]; then + # Suites a strict step owns ran in that step, not this batch. + python3 - "$shard_args" <<'PY' + import os + import sys + sys.path.insert(0, "scripts/ci") + from cmux_unit_test_shard import FOCUSED_GATE_SELECTORS + with open(sys.argv[1], "w", encoding="utf-8") as handle: + for selector in os.environ["CMUX_APP_HOST_UNIT_SELECTORS"].split(): + if selector not in FOCUSED_GATE_SELECTORS: + handle.write(f"-only-testing:{selector}\n") + PY + if [ ! -s "$shard_args" ]; then + echo "Every changed suite ran in its strict step; no shared batch." + return 0 + fi + else + python3 scripts/ci/cmux_unit_test_shard.py \ + --shard-index "$logical_shard" \ + --shard-total "$LOGICAL_SHARD_TOTAL" \ + --physical-shard-total "$PHYSICAL_SHARD_TOTAL" \ + ${reserve_args[@]+"${reserve_args[@]}"} \ + --output "$shard_args" || { + local shard_status=$? + echo "Failed to generate app-host unit-test batch ${logical_shard}/${LOGICAL_SHARD_TOTAL}" + return "$shard_status" + } + fi if [ ! -r "$shard_args" ]; then echo "Missing selector file for app-host unit-test batch ${logical_shard}/${LOGICAL_SHARD_TOTAL}" return 1 @@ -1936,7 +1976,7 @@ jobs: bun-version: "1.3.6" - name: Run agent notification semantics - if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) }} + if: ${{ (matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) || contains(inputs.unit_strict_steps, '|Run agent notification semantics|')) }} run: | set -euo pipefail # Reuse compile admission's product instead of rebuilding the app in diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 974af8b84279..ebeb30213a10 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -70,6 +70,8 @@ jobs: ghosttykit_release: ${{ steps.unchanged_inputs.outputs.compile_admitted == 'true' && 'false' || steps.linux_guards.outputs.ghosttykit_release }} full_suite: ${{ steps.suite.outputs.full_suite }} unit_suite: ${{ steps.suite.outputs.unit_suite }} + unit_selectors: ${{ steps.suite.outputs.unit_selectors }} + unit_strict_steps: ${{ steps.suite.outputs.unit_strict_steps }} coverage_gap: ${{ steps.suite.outputs.coverage_gap }} compile_admitted: ${{ steps.unchanged_inputs.outputs.compile_admitted == 'true' && 'true' || steps.admitted.outputs.compile_admitted }} source_tree: ${{ steps.source-identity.outputs.tree }} @@ -150,6 +152,11 @@ jobs: emit_all_areas exit 0 fi + # The changed lines of cmuxTests/, so choose_ci_suite.py can tell + # which declarations a test edit touched. Absent reads as "every + # line of each changed file". + git diff --no-renames -U0 "$BASE_SHA" "$MERGE_SHA" -- cmuxTests \ + > /tmp/cmux-ci-tests.diff || rm -f /tmp/cmux-ci-tests.diff if [ ! -s /tmp/cmux-ci-changed-files.txt ]; then echo "PR diff is empty; skipping product-area CI." @@ -465,6 +472,9 @@ jobs: if [ -f /tmp/cmux-ci-changed-files.txt ]; then files_args=(--files-from /tmp/cmux-ci-changed-files.txt) fi + if [ -f /tmp/cmux-ci-tests.diff ]; then + files_args+=(--diff-from /tmp/cmux-ci-tests.diff) + fi python3 scripts/ci/choose_ci_suite.py \ --event-name "$EVENT_NAME" \ --pull-request-policy "$PULL_REQUEST_POLICY" \ @@ -969,6 +979,8 @@ jobs: macos: ${{ needs.changes.outputs.macos }} full_suite: ${{ needs.changes.outputs.full_suite }} unit_suite: ${{ needs.changes.outputs.unit_suite }} + unit_selectors: ${{ needs.changes.outputs.unit_selectors }} + unit_strict_steps: ${{ needs.changes.outputs.unit_strict_steps }} swift_packages: ${{ needs.changes.outputs.swift_packages }} compile_admitted: ${{ needs.changes.outputs.compile_admitted }} release_build: ${{ needs.changes.outputs.release_build }} diff --git a/CLAUDE.md b/CLAUDE.md index e1991829a6e4..7b7af11e5c5e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -216,7 +216,10 @@ Add `full-ci` only when the user or agreed validation plan explicitly calls for the broad suite; state which additional lanes are needed and why. Normal PR CI can already run routed tests, including Swift package and CLI -wrapper checks, without `full-ci`. The label permits eligible app-host shards, +wrapper checks, without `full-ci`. A `cmuxTests/` diff runs the suites it +declares or extends on one app-host worker, with no label. `unit-ci` runs every +app-host suite across all seven workers; `full-ci` adds the other lanes on top. +Neither is needed to test the suites you edited. The label permits eligible app-host shards, lag builds, and other full-suite lanes; path routing, release routing, and job dependencies still apply. It does not request every repository test. Inspect actual executed tests on the current SHA: a green skipped job is not coverage. diff --git a/scripts/ci/choose_ci_suite.py b/scripts/ci/choose_ci_suite.py index 4193b60cb032..cd8c7d2a60ed 100755 --- a/scripts/ci/choose_ci_suite.py +++ b/scripts/ci/choose_ci_suite.py @@ -24,10 +24,21 @@ import argparse import json +import re import sys from collections.abc import Iterable from pathlib import Path +sys.path.insert(0, str(Path(__file__).resolve().parent)) +from test_impact import affected_suites # noqa: E402 +from cmux_unit_test_shard import ( # noqa: E402 + DEFAULT_TIMINGS_PATH, + FOCUSED_GATE_SELECTORS, + discover_selectors, + load_timings, + reweight_selectors, +) + COMPILE_ONLY_POLICY = "compile-only" FULL_SUITE_LABEL = "full-ci" SUITE_OPT_OUT_LABEL = "no-full-ci" @@ -45,6 +56,11 @@ UNJUDGED_BY_ANY_PR_JOB_PREFIXES = ("cmuxUITests/",) UNJUDGED_BY_COMPILE_PREFIXES = UNIT_JUDGED_PREFIXES + UNJUDGED_BY_ANY_PR_JOB_PREFIXES +# Measured serial test time a changed-suites run may hold. One runner executes +# it as a single batch, so it has to fit comfortably inside the batch timeout +# a normal shard's batch fits in; a larger diff takes all seven shards. +CHANGED_SUITES_BUDGET_MS = 10 * 60 * 1000 + def diff_needs_the_suite(paths: Iterable[str] | None) -> bool: """True when the diff contains changes compile admission cannot judge. @@ -99,6 +115,61 @@ def wants_unit_suite( return any(path.strip().startswith(UNIT_JUDGED_PREFIXES) for path in paths) +def strict_steps(workflow: str, suites: Iterable[str]) -> list[str] | None: + """Names of the app-host steps that run `suites` a strict step owns. + + Such a suite gets an app host and settings of its own from its step, so a + changed-suites run runs that step rather than putting the suite in its + shared batch. None when a selected strict suite has no step that names it. + """ + job = workflow[workflow.index("\n app-host-unit-tests:\n") :] + job = job[: re.search(r"\n [A-Za-z0-9_-]+:\n", job[1:]).start() + 1] + owners: dict[str, set[str]] = {} + for block in job.split("\n - name: ")[1:]: + name = block.split("\n", 1)[0].strip() + condition = re.search(r"^ if: (.*)$", block, re.M) + if condition is None or "_SHARD)" not in condition.group(1) or "!=" in condition.group(1): + continue + for selector in FOCUSED_GATE_SELECTORS: + if re.search(rf"\b{selector.split('/', 1)[1]}\b", block): + owners.setdefault(selector, set()).add(name) + names: set[str] = set() + for suite in suites: + if suite in FOCUSED_GATE_SELECTORS: + if suite not in owners: + return None + names |= owners[suite] + return sorted(names) + + +def changed_unit_selectors( + root: Path, paths: Iterable[str] | None, diff: str | None = None +) -> list[str]: + """Suite selectors for a unit run the diff selected, or [] for all of them. + + A pull request that edits a few tests needs those tests run, not the + other few thousand across seven shards. An empty answer keeps the full + unit suite: see test_impact.affected_suites(), strict_steps(), and a + shared batch whose measured time would not fit one worker's. + """ + if paths is None: + return [] + suites = affected_suites(root, [path.strip() for path in paths], diff) + if not suites: + return [] + workflow = (root / ".github/workflows/ci-macos.yml").read_text(encoding="utf-8") + if strict_steps(workflow, suites) is None: + return [] + wanted = {suite.split("/", 1)[1] for suite in suites} + selectors, _ = reweight_selectors(discover_selectors(root), load_timings(DEFAULT_TIMINGS_PATH)) + cost = sum( + selector.weight for selector in selectors if selector.identifier.split("/")[1] in wanted + ) + if cost > CHANGED_SUITES_BUDGET_MS: + return [] + return suites + + def labels_from_event(event_path: str | Path) -> list[str] | None: """Read the pull request labels captured in this workflow run's event payload.""" try: @@ -172,6 +243,11 @@ def main(argv: list[str]) -> int: "--files-from", help="changed paths, one per line; omit when the diff could not be read", ) + parser.add_argument( + "--diff-from", + help="`git diff -U0` of cmuxTests/; omit to count every line of a changed file", + ) + parser.add_argument("--root", type=Path, default=Path.cwd()) args = parser.parse_args(argv) labels = None @@ -189,12 +265,29 @@ def main(argv: list[str]) -> int: except (OSError, UnicodeError): paths = None + diff = None + if args.diff_from: + try: + diff = Path(args.diff_from).read_text(encoding="utf-8") + except (OSError, UnicodeError): + diff = None + full = wants_full_suite(args.event_name, args.pull_request_policy, labels) unit = wants_unit_suite(args.event_name, args.pull_request_policy, labels, paths) gap = coverage_gap(args.event_name, full, paths, labels, unit_suite=unit) + # Only a unit run the diff asked for narrows. `full-ci` and `unit-ci` are + # explicit requests for every suite. + asked_for_every_suite = full or UNIT_SUITE_LABEL in {label.strip() for label in labels or ()} + selectors = [] if not unit or asked_for_every_suite else changed_unit_selectors(args.root, paths, diff) + steps: list[str] = [] + if selectors: + workflow = (args.root / ".github/workflows/ci-macos.yml").read_text(encoding="utf-8") + steps = strict_steps(workflow, selectors) or [] lines = [ f"full_suite={'true' if full else 'false'}", f"unit_suite={'true' if unit else 'false'}", + f"unit_selectors={' '.join(selectors)}", + f"unit_strict_steps={''.join(f'|{step}' for step in steps) + '|' if steps else ''}", f"coverage_gap={'true' if gap else 'false'}", ] for line in lines: diff --git a/scripts/ci/test_impact.py b/scripts/ci/test_impact.py new file mode 100644 index 000000000000..9f022997ec5e --- /dev/null +++ b/scripts/ci/test_impact.py @@ -0,0 +1,296 @@ +#!/usr/bin/env python3 +"""Which cmuxTests/ suites a diff can change the behavior of. + +A pull request that edits a few tests needs those tests run, not every suite. +Suites run when a changed line sits inside them. A changed declaration another +file can call is a helper: every place that names it is found, and the +declaration around that place is changed in turn. A test method ends the +trail at its suite; a helper method continues it. + +The answer is None, meaning "run every suite", whenever it could be +incomplete: a changed file under cmuxTests/ that is not Swift, or a changed +declaration nothing can search for by name (a conformance, an operator). +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from pathlib import Path + +# A column-zero declaration. +TOP_LEVEL_RE = re.compile( + r"^(?:@[A-Za-z_][A-Za-z0-9_]*(?:\([^)]*\))?\s+)*" + r"((?:[a-z]+(?:\([a-z]+\))?\s+)*)" + r"(func|enum|struct|class|actor|protocol|extension|let|var|typealias)\s+" + r"([A-Za-z_][A-Za-z0-9_]*)" +) +# A member one level inside a top-level declaration. Name-less kinds (init, +# subscript, operators) leave the name group empty. +MEMBER_RE = re.compile( + r"^ (?:@[A-Za-z_][A-Za-z0-9_]*(?:\([^)]*\))?\s+)*" + r"((?:[a-z]+(?:\([a-z]+\))?\s+)*)" + r"(?:(func|var|let|enum|struct|class|actor|typealias|case)\s+([A-Za-z_][A-Za-z0-9_]*)" + r"|(init|subscript|func\s+[^A-Za-z_\s(]))" +) +IDENTIFIER_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") +HUNK_RE = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") +FILE_LOCAL = {"private", "fileprivate"} +SUITE_KINDS = {"class", "struct", "actor", "extension"} +# Kinds whose body holds members rather than statements. +CONTAINER_KINDS = {"class", "struct", "actor", "enum", "protocol", "extension"} +# XCTest calls these itself; nothing else names them. +XCTEST_HOOKS = {"setUp", "tearDown", "setUpWithError", "tearDownWithError", "invokeTest"} + + +@dataclass(frozen=True) +class Declaration: + start: int + end: int + name: str | None + suite: str | None + visible: bool + test: bool + # The type a member belongs to: callers elsewhere name it to reach one. + owner: str | None = None + # A non-suite declaration with tests inside, such as a container of + # nested Swift Testing suites, whose selectors this module cannot name. + holds_tests: bool = False + + +@dataclass(frozen=True) +class Outline: + top: tuple[Declaration, ...] + members: tuple[Declaration, ...] + + +def is_suite(kind: str, name: str) -> bool: + return kind in SUITE_KINDS and name.endswith("Tests") + + +def outline(lines: list[str]) -> Outline: + """Top-level declarations and their one-level members, with line ranges.""" + starts: list[tuple[int, str, str, str]] = [] + for number, line in enumerate(lines, start=1): + match = TOP_LEVEL_RE.match(line) + if match is not None: + starts.append((number, match.group(1), match.group(2), match.group(3))) + top: list[Declaration] = [] + members: list[Declaration] = [] + for index, (start, modifiers, kind, name) in enumerate(starts): + end = starts[index + 1][0] - 1 if index + 1 < len(starts) else len(lines) + suite = name if is_suite(kind, name) else None + file_local = bool(FILE_LOCAL & set(modifiers.split())) + top.append( + Declaration( + start, + end, + # An extension of another type adds members; the type's own + # name tells a search nothing about them. A conformance adds + # behavior no name leads to. + None if kind == "extension" and suite is None and ":" in lines[start - 1].split("{")[0] + else (name if kind != "extension" or suite else ""), + suite, + not file_local, + False, + holds_tests=suite is None + and any( + marker in text + for text in lines[start - 1 : end] + for marker in ("@Test", "@Suite", "XCTestCase") + ), + ) + ) + member_starts: list[tuple[int, re.Match[str]]] = [ + (number, match) + for number in range(start + 1, end + 1) + if kind in CONTAINER_KINDS + and (match := MEMBER_RE.match(lines[number - 1])) is not None + ] + for position, (member_start, match) in enumerate(member_starts): + member_end = ( + member_starts[position + 1][0] - 1 if position + 1 < len(member_starts) else end + ) + member_name = match.group(3) + # Only a suite's hooks and test methods are called by the test + # runner alone; a helper's `tearDown()` has callers. + test = member_name is not None and suite is not None and ( + member_name.startswith("test") + or member_name in XCTEST_HOOKS + or has_test_attribute(lines, member_start) + ) + members.append( + Declaration( + member_start, + member_end, + member_name, + suite, + not file_local and not (FILE_LOCAL & set(match.group(1).split())), + test, + # Extension members are called on values ("a".shouted) + # that never name the extended type. + None if kind == "extension" and suite is None else name, + ) + ) + return Outline(tuple(top), tuple(members)) + + +def has_test_attribute(lines: list[str], start: int) -> bool: + """Whether the member declared at `start` carries @Test, on its line or above.""" + if "@Test" in lines[start - 1]: + return True + # Walk up through attribute lines, including multi-line arguments, to the + # previous member's closing brace or a blank line. + for number in range(start - 1, max(start - 12, 0), -1): + text = lines[number - 1] + if not text.strip() or text.startswith(" }") or MEMBER_RE.match(text): + return False + if text.startswith(" @Test"): + return True + return False + + +def changed_lines(diff: str) -> dict[str, set[int]]: + """New-side line numbers each file's hunks touch, from `git diff -U0`.""" + changed: dict[str, set[int]] = {} + path: str | None = None + for line in diff.splitlines(): + if line.startswith("+++ "): + target = line[4:].strip() + path = target[2:] if target.startswith("b/") else None + if path is not None: + changed.setdefault(path, set()) + continue + match = HUNK_RE.match(line) + if match is None or path is None: + continue + first = int(match.group(1)) + count = int(match.group(2)) if match.group(2) is not None else 1 + # A pure deletion touches the declaration around where it was. + changed[path].update(range(first, first + count) if count else {max(first, 1)}) + return changed + + +def enclosing(outline_: Outline, line: int) -> tuple[Declaration | None, Declaration | None]: + top = next((item for item in outline_.top if item.start <= line <= item.end), None) + member = next((item for item in outline_.members if item.start <= line <= item.end), None) + return top, member + + +def affected_suites( + root: Path, paths: list[str], diff: str | None +) -> list[str] | None: + """cmuxTests/ suite selectors a diff affects; None to run every suite. + + `diff` is `git diff -U0` output for the same change. Without it, every + line of each changed file counts as changed. + """ + files: dict[str, list[str]] = {} + mentions: dict[str, set[str]] = {} + for source in sorted((root / "cmuxTests").glob("**/*.swift")): + relative = source.relative_to(root).as_posix() + try: + text = source.read_text(encoding="utf-8") + except (OSError, UnicodeError): + return None + files[relative] = text.splitlines() + for word in set(IDENTIFIER_RE.findall(text)): + mentions.setdefault(word, set()).add(relative) + outlines: dict[str, Outline] = {} + + def outline_of(path: str) -> Outline: + if path not in outlines: + outlines[path] = outline(files[path]) + return outlines[path] + + hunks = changed_lines(diff) if diff is not None else {} + suites: set[str] = set() + # (name, type that owns it). Another file reaches a member through its + # type's name, so that search stays in files that name both. + names: list[tuple[str, str | None]] = [] + + def touch(path: str, line: int) -> bool: + """Record what a changed or referencing line affects; False if untraceable.""" + top, member = enclosing(outline_of(path), line) + if top is None: + return True # imports, comments + if top.name is None or top.holds_tests: + return False # inside a conformance, or suites named nowhere here + if top.suite is not None: + suites.add(top.suite) + if member is not None and member.start > top.start: + if member.name is None: + # init, subscript or operator: trace through the type, which + # an extension of some other type does not name. + if not top.name: + return False + if top.visible: + names.append((top.name, None)) + elif member.visible and not member.test: + if member.owner is not None and top.suite is None: + # A helper type's member. App code may call it (a mock) + # and callers may hold an instance without naming it, so + # trace the type: whatever constructs, returns or stores + # one is affected. + names.append((member.owner, None)) + else: + names.append((member.name, member.owner)) + return True + if top.name is None: + return False + if top.visible and top.name: + names.append((top.name, None)) + return True + + for path in (path.strip() for path in paths): + if not path.startswith("cmuxTests/"): + continue + if not (root / path).exists(): + # Deleted: whatever used it changed in this diff too, or no + # longer compiles. + continue + if path not in files: + return None + text = files[path] + whole = range(1, len(text) + 1) + lines = hunks.get(path) if diff is not None else None + first_declaration = min((item.start for item in outline_of(path).top), default=len(text) + 1) + if not lines or any( + line < first_declaration and text[line - 1].strip() and not text[line - 1].lstrip().startswith("//") + for line in lines + ): + # No line information, or an import or other file-level line: + # the whole file changed. + lines = set(whole) + for line in sorted(lines): + if not touch(path, line): + return None + if text[line - 1].lstrip().startswith(("@", "#", "///")): + # An attribute, directive or doc comment belongs to the + # declaration it precedes, not the one above it. + following = next( + ( + number + for number in range(line + 1, min(line + 20, len(text)) + 1) + if TOP_LEVEL_RE.match(text[number - 1]) or MEMBER_RE.match(text[number - 1]) + ), + None, + ) + if following is not None and not touch(path, following): + return None + + searched: set[tuple[str, str | None]] = set() + while names: + name, owner = names.pop() + if (name, owner) in searched: + continue + searched.add((name, owner)) + pattern = re.compile(rf"\b{re.escape(name)}\b") + scope = mentions.get(name, set()) + if owner is not None: + scope = scope & mentions.get(owner, set()) + for path in sorted(scope): + for number, text in enumerate(files[path], start=1): + if pattern.search(text) and not touch(path, number): + return None + return sorted(f"cmuxTests/{suite}" for suite in suites) diff --git a/tests/test_ci_app_host_home_isolation.py b/tests/test_ci_app_host_home_isolation.py index ffeab78a73b9..a1715f61fc32 100644 --- a/tests/test_ci_app_host_home_isolation.py +++ b/tests/test_ci_app_host_home_isolation.py @@ -170,9 +170,27 @@ def acceptance_gate_problem(condition: object, preparation_id: str) -> str: expression = condition.strip() if expression.startswith("${{") and expression.endswith("}}"): expression = expression[3:-2] - if "||" in expression: - return "must not offer an alternative to its gates" - terms = {"".join(term.split()) for term in expression.split("&&")} + # Split on the top-level `&&` only. An `||` inside a parenthesized term + # chooses which worker runs the step; one outside offers a way around + # the gates themselves. + top_level: list[str] = [] + depth = 0 + term = "" + index = 0 + while index < len(expression): + character = expression[index] + depth += {"(": 1, ")": -1}.get(character, 0) + if depth == 0 and expression.startswith("||", index): + return "must not offer an alternative to its gates" + if depth == 0 and expression.startswith("&&", index): + top_level.append(term) + term = "" + index += 2 + continue + term += character + index += 1 + top_level.append(term) + terms = {"".join(term.split()) for term in top_level} if "always()" in terms: return "must not run after a cancelled job" if "!cancelled()" not in terms: @@ -400,6 +418,11 @@ def main() -> int: + preparation_id + ".outcome == 'success' || matrix.shard == 1 }}" ), + "a parenthesized alternative to a gate": ( + "${{ !cancelled() && (steps." + + preparation_id + + ".outcome == 'success' || matrix.shard == 1) }}" + ), }.items(): if not acceptance_gate_problem(fixture, preparation_id): raise SystemExit(f"FAIL: acceptance gate guard must reject {rejected}") diff --git a/tests/test_ci_change_areas.py b/tests/test_ci_change_areas.py index 499635dac614..72896324f704 100755 --- a/tests/test_ci_change_areas.py +++ b/tests/test_ci_change_areas.py @@ -3617,6 +3617,210 @@ def test_a_cmux_tests_diff_selects_the_unit_tests_without_a_label() -> None: # An unreadable diff runs the unit tests rather than guessing. assert wants_unit_suite("pull_request", "compile-only", [], None) is True +def test_a_diff_that_edits_a_few_suites_runs_only_those_suites() -> None: + sys.path.insert(0, str(ROOT / "scripts/ci")) + from choose_ci_suite import changed_unit_selectors, strict_steps + from test_impact import affected_suites + + def hunk(path: str, line: int, count: int = 1) -> str: + return f"--- a/{path}\n+++ b/{path}\n@@ -{line},{count} +{line},{count} @@\n" + + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + tests = root / "cmuxTests" + tests.mkdir() + (root / ".github/workflows").mkdir(parents=True) + (root / ".github/workflows/ci-macos.yml").write_text(MACOS_WORKFLOW.read_text(encoding="utf-8")) + files = { + # 1 import, 2 class, 3 helper, 4 body, 5 test, 6 body, 7-8 @Test lines, 9 test + "AlphaTests.swift": ( + "import XCTest\n" + "final class AlphaTests: XCTestCase {\n" + " static func makeModel() -> Int {\n" + " 1\n" + " }\n" + " func testA() { _ = sharedHelper() }\n" + " @Test(\n" + " arguments: [1])\n" + " func modernName(value: Int) {}\n" + "}\n" + ), + "BetaTests.swift": ( + "import XCTest\n" + "final class BetaTests: XCTestCase {\n" + " func testB() { _ = AlphaTests.makeModel() }\n" + "}\n" + ), + "GammaTests.swift": ( + "import XCTest\n" + "final class GammaTests: XCTestCase {\n" + " func testG() { _ = makeModel() }\n" + " func makeModel() -> Int { 2 }\n" + "}\n" + ), + "Helper.swift": "func sharedHelper() -> Int {\n wrapped()\n}\nfunc wrapped() -> Int { 0 }\n", + "UsesHelperTests.swift": ( + "import XCTest\n" + "final class UsesHelperTests: XCTestCase {\n" + " func testU() { _ = sharedHelper() }\n" + "}\n" + ), + "StringExtras.swift": "extension String {\n var shouted: String { uppercased() }\n}\n", + "ShoutTests.swift": ( + "import XCTest\n" + "final class ShoutTests: XCTestCase {\n" + " func testS() { _ = \"a\".shouted }\n" + "}\n" + ), + "Conformances.swift": "extension Int: @retroactive Identifiable {\n public var id: Int { self }\n}\n", + "FeedCoordinatorTests.swift": ( + "import Testing\n@Suite struct FeedCoordinatorTests {\n @Test func testF() {}\n}\n" + ), + "Fixture.json": "{}\n", + # 1 class, 2 member, 3 body, 4 close, 5 close, 6 factory + "Recorder.swift": ( + "final class Recorder: IntentRecording {\n" + " func record(_ intent: Int) {\n" + " intents.append(intent)\n" + " }\n" + "}\n" + "func makeRecorder() -> Recorder { Recorder() }\n" + ), + "BuildsRecorderTests.swift": ( + "import XCTest\n" + "final class BuildsRecorderTests: XCTestCase {\n" + " func testR() { _ = Recorder() }\n" + "}\n" + ), + "FactoryTests.swift": ( + "import XCTest\n" + "final class FactoryTests: XCTestCase {\n" + " func testF() { makeRecorder().record(1) }\n" + "}\n" + ), + # 1 struct, 2 tearDown, 3 body, 4 close, 5 close + "Harness.swift": ( + "struct Harness {\n" + " func tearDown() {\n" + " stop()\n" + " }\n" + "}\n" + ), + "HarnessTests.swift": ( + "import XCTest\n" + "final class HarnessTests: XCTestCase {\n" + " func testH() { Harness().tearDown() }\n" + "}\n" + ), + # 1 func, 2 close, 3 attribute, 4 func, 5 close + "Plain.swift": "func first() -> Int {\n}\n@MainActor\nfunc plain() {\n}\n", + "PlainTests.swift": ( + "import XCTest\n" + "final class PlainTests: XCTestCase {\n" + " func testP() { plain() }\n" + "}\n" + ), + "Container.swift": ( + "enum SettingsSuites {}\n" + "extension SettingsSuites {\n" + " @Suite struct ChromeTests {\n" + " @Test func chrome() {}\n" + " }\n" + "}\n" + ), + } + for name, text in files.items(): + (tests / name).write_text(text) + + def affected(path: str, line: int | None = None) -> list[str] | None: + full = f"cmuxTests/{path}" + return affected_suites(root, [full], None if line is None else hunk(full, line)) + + # A test method's edit runs its suite and nothing else, including a + # Swift Testing method whose @Test sits above a multi-line argument. + assert affected("AlphaTests.swift", 6) == ["cmuxTests/AlphaTests"] + assert affected("AlphaTests.swift", 9) == ["cmuxTests/AlphaTests"] + # A suite's helper reaches the suites that name the suite, not every + # file with a method of the same name. + assert affected("AlphaTests.swift", 4) == ["cmuxTests/AlphaTests", "cmuxTests/BetaTests"] + # A top-level helper is traced through the helpers that call it. + assert affected("Helper.swift", 4) == ["cmuxTests/AlphaTests", "cmuxTests/UsesHelperTests"] + # Members added to another type are traced by their names. + assert affected("StringExtras.swift", 2) == ["cmuxTests/ShoutTests"] + # A conformance has no name to search for. + assert affected("Conformances.swift", 2) is None + # A helper type's member traces the type: a suite that only builds + # the mock for app code to call runs, and so does one that reaches it + # through a factory without naming it. + assert affected("Recorder.swift", 3) == ["cmuxTests/BuildsRecorderTests", "cmuxTests/FactoryTests"] + # A helper's `tearDown()` is a helper, not a hook only XCTest calls. + assert affected("Harness.swift", 3) == ["cmuxTests/HarnessTests"] + # An attribute line belongs to the declaration below it. + assert affected("Plain.swift", 3) == ["cmuxTests/PlainTests"] + # Suites nested in a container are named nowhere here: run everything. + assert affected("Container.swift", 4) is None + # An import is a change to the whole file. + assert affected("AlphaTests.swift", 1) == ["cmuxTests/AlphaTests", "cmuxTests/BetaTests"] + # Without line information every line of the file counts. + assert affected("AlphaTests.swift") == ["cmuxTests/AlphaTests", "cmuxTests/BetaTests"] + # Non-Swift inputs run everything; a deleted file leaves nothing. + assert affected_suites(root, ["cmuxTests/Fixture.json"], None) is None + assert affected_suites(root, ["cmuxTests/GoneTests.swift"], None) == [] + assert changed_unit_selectors(root, None) == [] + assert changed_unit_selectors(root, ["cmuxTests/GoneTests.swift"]) == [] + # A suite a strict step owns runs through that step, on the same worker. + feed = ["cmuxTests/FeedCoordinatorTests.swift"] + assert changed_unit_selectors(root, feed) == ["cmuxTests/FeedCoordinatorTests"] + workflow = MACOS_WORKFLOW.read_text(encoding="utf-8") + assert strict_steps(workflow, ["cmuxTests/FeedCoordinatorTests"]) == ["Run Pi Feed ownership regressions"] + assert strict_steps(workflow, ["cmuxTests/AlphaTests"]) == [] + + +def test_changed_suites_run_on_one_worker_and_labels_still_run_everything() -> None: + workflow = yaml.safe_load(MACOS_WORKFLOW.read_text(encoding="utf-8")) + job = workflow["jobs"]["app-host-unit-tests"] + matrix = job["strategy"]["matrix"]["shard"] + assert "'[8]'" in matrix and "'[1, 2, 3, 4, 5, 6, 7]'" in matrix, matrix + assert job["env"]["CMUX_APP_HOST_UNIT_SELECTORS"] == "${{ inputs.unit_selectors }}" + # Shard 8 must own none of the strict steps the numbered shards run. + owners = {key: value for key, value in job["env"].items() if key.endswith("_SHARD")} + assert "8" not in owners.values(), owners + # Every strict suite has a step, and each such step runs when selected. + from choose_ci_suite import strict_steps + from cmux_unit_test_shard import FOCUSED_GATE_SELECTORS + + text = MACOS_WORKFLOW.read_text(encoding="utf-8") + owners = strict_steps(text, sorted(FOCUSED_GATE_SELECTORS)) + assert owners, "a strict suite has no step that runs it" + for step in job["steps"]: + if step.get("name") in owners: + assert f"contains(inputs.unit_strict_steps, '|{step['name']}|')" in step["if"], step["name"] + assert yaml.safe_load(CI_WORKFLOW.read_text(encoding="utf-8"))["jobs"]["macos"]["with"]["unit_strict_steps"] == "${{ needs.changes.outputs.unit_strict_steps }}" + ci = yaml.safe_load(CI_WORKFLOW.read_text(encoding="utf-8")) + assert ci["jobs"]["macos"]["with"]["unit_selectors"] == "${{ needs.changes.outputs.unit_selectors }}" + + script = ROOT / "scripts/ci/choose_ci_suite.py" + with tempfile.TemporaryDirectory() as directory: + changed = Path(directory) / "changed.txt" + changed.write_text("cmuxTests/TerminalTabIconRegressionTests.swift\n") + labels = Path(directory) / "labels.txt" + + def selectors(label: str) -> str: + labels.write_text(label) + run = subprocess.run( + [sys.executable, str(script), "--event-name", "pull_request", + "--pull-request-policy", "compile-only", "--labels-file", str(labels), + "--files-from", str(changed), "--root", str(ROOT)], + capture_output=True, text=True, check=True, + ) + return next(line for line in run.stdout.splitlines() if line.startswith("unit_selectors=")) + + assert selectors("") == "unit_selectors=cmuxTests/TerminalTabIconRegressionTests" + # An explicit request for every suite is honored. + assert selectors("unit-ci\n") == "unit_selectors=" + assert selectors("full-ci\n") == "unit_selectors=" + + def test_the_unit_tier_closes_only_the_gap_its_job_can_judge() -> None: sys.path.insert(0, str(ROOT / "scripts/ci")) from choose_ci_suite import coverage_gap diff --git a/tests/test_ci_cmux_unit_test_shard.py b/tests/test_ci_cmux_unit_test_shard.py index 3590923d2185..6c87df7d3fcf 100644 --- a/tests/test_ci_cmux_unit_test_shard.py +++ b/tests/test_ci_cmux_unit_test_shard.py @@ -624,7 +624,7 @@ def check_global_search_has_dedicated_consumer() -> int: print("FAIL: app-host-unit-tests job missing") return 1 job = match.group(1) - if "shard: [1, 2, 3, 4, 5, 6, 7]" not in job: + if "'[1, 2, 3, 4, 5, 6, 7]'" not in job: print("FAIL: app-host matrix must include the dedicated seventh consumer") return 1 if 'CMUX_APP_HOST_GLOBAL_SEARCH_SHARD: "7"' not in job: