diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a6c85c186a04..6eae56c3eeec 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,8 +69,9 @@ jobs: linux_guard_source: ${{ steps.linux_guards.outputs.linux_guard_source }} ghosttykit_release: ${{ steps.unchanged_inputs.outputs.compile_admitted == 'true' && 'false' || steps.linux_guards.outputs.ghosttykit_release }} full_suite: ${{ steps.suite.outputs.full_suite }} - # A consumer canary (choose_ci_suite.py) only rides on a compile this run - # pays for; when either reuse check finds one, it is dropped. + # A canary run (choose_ci_suite.py: the consumer canary, or suites only + # the reverse test impact selector reached) only rides on a compile this + # run pays for; when either reuse check finds one, it is dropped. unit_suite: ${{ steps.suite.outputs.unit_canary == 'true' && (steps.unchanged_inputs.outputs.compile_admitted == 'true' || steps.admitted.outputs.compile_admitted == 'true') && 'false' || steps.suite.outputs.unit_suite }} unit_selectors: ${{ !(steps.suite.outputs.unit_canary == 'true' && (steps.unchanged_inputs.outputs.compile_admitted == 'true' || steps.admitted.outputs.compile_admitted == 'true')) && steps.suite.outputs.unit_selectors || '' }} unit_strict_steps: ${{ steps.suite.outputs.unit_strict_steps }} @@ -190,6 +191,10 @@ jobs: # Absent reads as "every line of each changed file". git diff --no-renames -U0 "$BASE_SHA" "$MERGE_SHA" -- cmuxTests .github/workflows/ci-macos.yml \ > /tmp/cmux-ci-tests.diff || rm -f /tmp/cmux-ci-tests.diff + # The changed lines of app code, so choose_ci_suite.py can add the + # suites whose tests mention what changed (reverse_test_impact.py). + git diff --no-renames -U0 "$BASE_SHA" "$MERGE_SHA" -- Sources Packages/macOS Packages/Shared CLI \ + > /tmp/cmux-ci-app.diff || rm -f /tmp/cmux-ci-app.diff if [ ! -s /tmp/cmux-ci-changed-files.txt ]; then echo "PR diff is empty; skipping product-area CI." @@ -592,6 +597,9 @@ jobs: if [ -f /tmp/cmux-ci-tests.diff ]; then files_args+=(--diff-from /tmp/cmux-ci-tests.diff) fi + if [ -f /tmp/cmux-ci-app.diff ]; then + files_args+=(--app-diff-from /tmp/cmux-ci-app.diff) + fi python3 scripts/ci/choose_ci_suite.py \ --event-name "$EVENT_NAME" \ --pull-request-policy "$PULL_REQUEST_POLICY" \ @@ -903,11 +911,11 @@ jobs: python3 scripts/ci/find_admitted_build.py --repository "$GITHUB_REPOSITORY" --branch "$BRANCH" \ --fingerprint "$FINGERPRINT" --current-run-id "$GITHUB_RUN_ID" --github-output "$GITHUB_OUTPUT" - # Report only. A pull request that changes app code but no cmuxTests/ file - # runs no app-host behavior test today; reverse_test_impact.py names the - # suites that could observe the change and what they would cost. The answer - # goes to the step summary and a 14-day artifact so its recall can be - # measured against later failures on main. It is its own job so it never + # Report only. choose_ci_suite.py in `changes` already adds the suites + # reverse_test_impact.py names to the changed-suites run; this job records + # the full answer, what was considered and what it would cost, in the step + # summary and a 14-day artifact so its recall can be measured against later + # failures on main. It is its own job so it never # delays routing: nothing needs it, ci-status does not wait for it, it has # no outputs, and it cannot fail the run (continue-on-error at job level). # diff --git a/scripts/ci/choose_ci_suite.py b/scripts/ci/choose_ci_suite.py index fef29872fe8c..ce134d244914 100755 --- a/scripts/ci/choose_ci_suite.py +++ b/scripts/ci/choose_ci_suite.py @@ -232,6 +232,62 @@ def changed_unit_selectors( return suites +def reverse_unit_selectors( + root: Path, paths: Iterable[str] | None, app_diff: str | None, already: list[str] +) -> list[str]: + """Suites that could observe an app-source change, within what the budget has left. + + A pull request that changes Sources/ or a macOS/Shared package without + touching cmuxTests/ otherwise runs no behavior test. reverse_test_impact.py + names the suites whose tests mention what the diff changed; this keeps the + ones that fit beside `already` in one changed-suites run. It only adds: + anything it cannot judge (no diff, a selector error) adds nothing, and a + suite that would push the run past its budget or out of the changed-suites + lane is left out rather than turning the run into seven shards. Only + suites the shared batch discovers are added: the selector also names + helper types in cmuxTests/, and a selector that matches no test fails the + run. A suite a strict step owns is left out, since that step runs apart + from the budget. Suites with entries in app-host-known-failures.json are + left out too: a known failure that happens to pass fails a changed-suites + run, which is right for a suite the pull request edited and wrong for one + it only reached. + """ + if paths is None or app_diff is None or not app_diff.strip(): + return [] + try: + import reverse_test_impact as reverse + + if not any(reverse.is_app_path(path.strip()) for path in paths): + return [] + selection = reverse.select(reverse.read_root(root), app_diff) + if not selection.reached: + return [] + timings = load_timings(DEFAULT_TIMINGS_PATH) + default_ms = (timings or {}).get("default_test_ms", reverse.FALLBACK_TEST_MS) + costs = reverse.suite_costs(root, timings) + spent = sum(costs.get(selector.split("/", 1)[1], default_ms) for selector in already) + if spent >= CHANGED_SUITES_BUDGET_MS: + return [] + catalog = json.loads((root / "scripts/ci/app-host-known-failures.json").read_text(encoding="utf-8")) + known = {identifier.split("/", 1)[0] for identifier in catalog.get("tests", {})} + workflow = (root / ".github/workflows/ci-macos.yml").read_text(encoding="utf-8") + batch_suites = {selector.identifier.split("/")[1] for selector in discover_selectors(root)} + data = reverse.report(selection, costs, default_ms, CHANGED_SUITES_BUDGET_MS - spent) + chosen: list[str] = [] + for suite in data["would_run"]: + selector = f"cmuxTests/{suite}" + if suite in known or suite not in batch_suites or selector in already: + continue + steps = strict_steps(workflow, already + chosen) + if strict_steps(workflow, already + chosen + [selector]) != steps: + continue + chosen.append(selector) + return chosen + except Exception as error: # an addition only: never the reason a run fails + print(f"::warning::Reverse test impact selection failed: {error!r}", file=sys.stderr) + return [] + + def job_lines(workflow: str, job: str) -> range | None: """1-based line numbers of `job` in a workflow's text, header included.""" lines = workflow.splitlines() @@ -404,6 +460,10 @@ def main(argv: list[str]) -> int: "--diff-from", help="`git diff -U0` of cmuxTests/ and ci-macos.yml; omit to count every line of a changed file", ) + parser.add_argument( + "--app-diff-from", + help="`git diff -U0` of Sources/, Packages/ and CLI/; adds the suites that could observe it", + ) parser.add_argument("--root", type=Path, default=Path.cwd()) args = parser.parse_args(argv) @@ -429,6 +489,13 @@ def main(argv: list[str]) -> int: except (OSError, UnicodeError): diff = None + app_diff = None + if args.app_diff_from: + try: + app_diff = Path(args.app_diff_from).read_text(encoding="utf-8", errors="replace") + except OSError: + app_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) @@ -437,6 +504,25 @@ def main(argv: list[str]) -> int: 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) canary = False + reached: list[str] = [] + # A narrowed run (or none yet) also takes the suites that could observe + # the app-source change; an empty `selectors` under `unit` is already + # every suite. + if not asked_for_every_suite and (selectors or not unit): + reached = reverse_unit_selectors(args.root, paths, app_diff, selectors) + if reached: + # Alone, these ride on a compile this run pays for, like the + # consumer canary: a re-push of admitted inputs reuses the build + # and drops them rather than compiling again. + canary = not unit + if canary: + # Keep the consumer canary a consumer edit would have taken. + selectors = [ + selector for selector in consumer_canary_selectors(args.root, paths, diff) + if selector not in reached + ] + selectors = selectors + reached + unit = True if not unit: # Nothing else asked for the unit tests, so a consumer edit takes the # one-suite canary rather than seven shards. ci.yml drops it again when @@ -447,7 +533,9 @@ def main(argv: list[str]) -> int: if selectors: workflow = (args.root / ".github/workflows/ci-macos.yml").read_text(encoding="utf-8") steps = strict_steps(workflow, selectors) or [] - in_admission = runs_in_admission(args.root, paths, diff, selectors, steps, canary) + # Suites reached this way can fill the whole budget; they take the + # changed-suites worker rather than holding compile admission. + in_admission = runs_in_admission(args.root, paths, diff, selectors, steps, canary or bool(reached)) lines = [ f"full_suite={'true' if full else 'false'}", f"unit_suite={'true' if unit else 'false'}", diff --git a/tests/test_ci_change_areas.py b/tests/test_ci_change_areas.py index 0e937718295c..a4a1978f0723 100755 --- a/tests/test_ci_change_areas.py +++ b/tests/test_ci_change_areas.py @@ -4809,6 +4809,59 @@ def outputs(paths: list[str], hunks: str | None = None) -> dict[str, str]: assert result["unit_in_admission"] == "false", result +def test_an_app_source_diff_runs_the_suites_that_mention_what_it_changed() -> None: + """#12822 changed AgentQuitProcessOwnership in Sources/ and main broke. + + A pull request that changes app code runs the suites whose tests mention + the changed declaration, on the changed-suites run, instead of no behavior + test at all. Without a readable app diff it adds nothing. + """ + script = ROOT / "scripts/ci/choose_ci_suite.py" + path = "Sources/App/AgentQuitProcessOwnership.swift" + declaration = next( + number for number, line in enumerate((ROOT / path).read_text(encoding="utf-8").splitlines(), 1) + if line.startswith("struct AgentQuitProcessOwnership") + ) + with tempfile.TemporaryDirectory() as directory: + changed = Path(directory) / "changed.txt" + changed.write_text(f"{path}\n") + labels = Path(directory) / "labels.txt" + labels.write_text("") + app_diff = Path(directory) / "app.diff" + + def outputs(hunks: str | None) -> dict[str, str]: + extra = [] + if hunks is not None: + app_diff.write_text(hunks) + extra = ["--app-diff-from", str(app_diff)] + 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), *extra], + capture_output=True, text=True, check=True, + ) + return dict(line.split("=", 1) for line in run.stdout.splitlines()) + + result = outputs(f"--- a/{path}\n+++ b/{path}\n@@ -{declaration},1 +{declaration},1 @@\n") + assert result["unit_suite"] == "true", result + # Like the consumer canary, these ride only on a compile the run pays + # for, and take the changed-suites worker rather than admission. + assert result["unit_canary"] == "true", result + assert result["unit_in_admission"] == "false", result + assert "cmuxTests/AgentQuitOwnershipTests" in result["unit_selectors"].split(), result + # Only suites the shared batch runs: the selector also names helper + # types, and a selector matching no test fails the run. + sys.path.insert(0, str(ROOT / "scripts/ci")) + from cmux_unit_test_shard import discover_selectors + batch = {f"cmuxTests/{s.identifier.split('/')[1]}" for s in discover_selectors(ROOT)} + assert set(result["unit_selectors"].split()) <= batch, set(result["unit_selectors"].split()) - batch + assert result["unit_strict_steps"] == "", result + + for unreadable in (None, ""): + result = outputs(unreadable) + assert "cmuxTests/AgentQuitOwnershipTests" not in result["unit_selectors"].split(), result + + def test_compile_admission_runs_changed_suites_that_need_no_worker() -> None: """A few changed suites run on the runner that compiled them.