From 45371b9ec3c6eac0f9781657fc7c5686a7d6f314 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 25 Sep 2026 05:32:49 -0400 Subject: [PATCH 1/3] test: a shard layout edit must run every app-host unit shard #14393 rebalanced the shards, took the one-suite consumer canary, and merged; main then failed four suites that only fail in the new order. Co-Authored-By: Claude Opus 5.5 --- tests/test_ci_change_areas.py | 82 +++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/tests/test_ci_change_areas.py b/tests/test_ci_change_areas.py index b2399893084a..eaabd33d09ef 100755 --- a/tests/test_ci_change_areas.py +++ b/tests/test_ci_change_areas.py @@ -5114,6 +5114,88 @@ def outputs(paths: list[str], label: str = "") -> dict[str, str]: f"${{{{ !({dropped}) && steps.suite.outputs.unit_selectors || '' }}}}", changes["outputs"]["unit_selectors"] +def test_a_shard_layout_edit_runs_every_app_host_unit_shard() -> None: + """A new shard layout puts suites in a new order, and only running all of it shows that. + + #14393 rebalanced the shards from measured timings, took the one-suite + consumer canary, and merged; main then failed four suites that only fail + in the new order (run 36101756298). + """ + sys.path.insert(0, str(ROOT / "scripts/ci")) + from choose_ci_suite import SHARD_LAYOUT_PATHS, shard_layout_changed + + workflow_path = ".github/workflows/ci-macos.yml" + lines = MACOS_WORKFLOW.read_text(encoding="utf-8").splitlines() + job_start = lines.index(" app-host-unit-tests:") + 1 + + def line_of(prefix: str) -> int: + return next(number for number, text in enumerate(lines[job_start:], start=job_start + 1) + if text.startswith(prefix)) + + def hunk(line: int, count: int = 1) -> str: + return f"--- a/{workflow_path}\n+++ b/{workflow_path}\n@@ -{line},{count} +{line},{count} @@\n" + + for path in SHARD_LAYOUT_PATHS[1:]: + assert (ROOT / path).is_file(), path + assert shard_layout_changed(ROOT, [path], None), path + # The generator runs in no CI job; its layout change arrives as the JSON. + assert "scripts/ci/generate_test_timings.py" not in SHARD_LAYOUT_PATHS + # Other consumer scripts keep the canary. + assert not shard_layout_changed(ROOT, ["scripts/ci/app_host_test_products.py"], None) + assert not shard_layout_changed(ROOT, ["Sources/Workspace.swift"], None) + assert not shard_layout_changed(ROOT, None, None) + # The shards' matrix, and the env that places strict steps and reserves + # their time on a worker, are the layout. + for prefix in (' {"shard": 3},', " strategy:", " CMUX_APP_HOST_RESERVED_WALL_SECONDS:", + " CMUX_APP_HOST_GLOBAL_SEARCH_SHARD:", " CMUX_APP_HOST_FOCUSED_REGRESSION_B_SHARD:"): + assert shard_layout_changed(ROOT, [workflow_path], hunk(line_of(prefix))), prefix + # The rest of the job, and other jobs, are not. + for prefix in (" CMUX_UNIT_TEST_TIMEOUT_SECONDS:", " timeout-minutes:", " runs-on:"): + assert not shard_layout_changed(ROOT, [workflow_path], hunk(line_of(prefix))), prefix + compile_line = lines.index(" macos-compile-admission:") + 2 + assert not shard_layout_changed(ROOT, [workflow_path], hunk(compile_line)) + # An edit nothing can place counts. + assert shard_layout_changed(ROOT, [workflow_path], None) + + script = ROOT / "scripts/ci/choose_ci_suite.py" + with tempfile.TemporaryDirectory() as directory: + changed = Path(directory) / "changed.txt" + labels = Path(directory) / "labels.txt" + diff = Path(directory) / "tests.diff" + + def outputs(paths: list[str], hunks: str = "", label: str = "") -> dict[str, str]: + changed.write_text("".join(f"{path}\n" for path in paths)) + labels.write_text(label) + diff.write_text(hunks) + run = subprocess.run( + [sys.executable, str(script), "--event-name", "pull_request", + "--pull-request-policy", "compile-only", "--labels-file", str(labels), + "--files-from", str(changed), "--diff-from", str(diff), "--root", str(ROOT)], + capture_output=True, text=True, check=True, + ) + return dict(line.split("=", 1) for line in run.stdout.splitlines()) + + every_shard = {"full_suite": "false", "unit_suite": "true", "unit_selectors": "", + "unit_strict_steps": "", "unit_canary": "false", "unit_in_admission": "false"} + # #14393's files, with and without the workflow hunk. + rebalance = ["scripts/ci/cmux-unit-test-timings.json", "scripts/ci/cmux_unit_test_shard.py", + "scripts/ci/generate_test_timings.py", "scripts/ci/run-app-host-unit-batches.sh"] + reserved = hunk(line_of(" CMUX_APP_HOST_RESERVED_WALL_SECONDS:")) + for paths, hunks in ( + (rebalance, ""), + (rebalance + [workflow_path], reserved), + ([workflow_path], reserved), + (["scripts/ci/cmux-unit-test-timings.json"], ""), + # An edited suite does not narrow a layout change to that suite. + (["scripts/ci/cmux-unit-test-timings.json", "cmuxTests/TerminalTabIconRegressionTests.swift"], ""), + ): + result = outputs(paths, hunks) + assert {key: result[key] for key in every_shard} == every_shard, (paths, result) + # A consumer hunk elsewhere in the job keeps the one-suite canary. + other = outputs([workflow_path], hunk(line_of(" CMUX_UNIT_TEST_TIMEOUT_SECONDS:"))) + assert (other["unit_selectors"], other["unit_canary"]) == ("cmuxTests/CmuxSSHURLRequestTests", "true"), other + + 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 From 4bd6243b4566a7eaa24c5e2f6a41c47b11cb16fc Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 25 Sep 2026 05:32:51 -0400 Subject: [PATCH 2/3] ci: run every app-host unit shard when a PR changes the shard layout The timings file, the sharder, the batch runner, and the app-host job's matrix and shard env decide which suites share a worker and in what order. Only running all seven shards shows order dependence; the one-suite consumer canary cannot. These now select the unit suite with no narrowing, as unit-ci does, without the rest of the full suite. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 4 ++- scripts/ci/choose_ci_suite.py | 67 +++++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b0bf2e29fe27..0e55082fe845 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -144,7 +144,9 @@ declares or extends, and an app-source diff runs the suites whose tests mention what it changed (`reverse_test_impact.py`, #14418), in one changed-suites batch with no label (edited suites over its budget take all seven shards). `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. No PR job runs +top. Neither is needed to test the suites you edited. A change to how the suites +are laid out over the workers (the timings file, the sharder, the batch runner, or +the job's matrix and shard env) runs every app-host suite on its own. No PR job runs `cmuxUITests/`; `no-full-ci` records a deliberate skip for `suite-coverage`. 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 diff --git a/scripts/ci/choose_ci_suite.py b/scripts/ci/choose_ci_suite.py index ce134d244914..23ee6e747efc 100755 --- a/scripts/ci/choose_ci_suite.py +++ b/scripts/ci/choose_ci_suite.py @@ -104,6 +104,20 @@ # executed and were accounted for, which is what a consumer edit can break. CONSUMER_CANARY_SELECTOR = "cmuxTests/CmuxSSHURLRequestTests" +# What decides which suites share a worker and in what order they run. A new +# layout can put one suite after another that leaves state behind, and only +# running every shard shows that: #14393 took the canary, merged, and main +# failed four suites that only fail in the new order. These run every unit +# suite, as `unit-ci` does, but not the rest of the full suite. +# generate_test_timings.py is left out: no CI job runs it, and its layout +# change arrives as the timings file it writes. +SHARD_LAYOUT_PATHS = ( + MACOS_WORKFLOW_PATH, # only shard_layout_lines() count + "scripts/ci/cmux-unit-test-timings.json", + "scripts/ci/cmux_unit_test_shard.py", + "scripts/ci/run-app-host-unit-batches.sh", +) + def diff_needs_the_suite(paths: Iterable[str] | None) -> bool: """True when the diff contains changes compile admission cannot judge. @@ -361,6 +375,51 @@ def consumer_canary_selectors( return [] +def shard_layout_lines(workflow: str) -> set[int]: + """1-based lines of `app-host unit tests` that lay out its shards. + + Its `strategy:` block (the shard matrix) and the job env that places a + strict step on a shard or reserves its time there. + """ + job = job_lines(workflow, APP_HOST_CONSUMER_JOB) + if job is None: + return set() + lines = workflow.splitlines() + layout: set[int] = set() + in_strategy = False + for number in job: + text = lines[number - 1] + if re.match(r"^ [A-Za-z_-]+:", text): + in_strategy = text.startswith(" strategy:") + if in_strategy or re.match(r"^ CMUX_APP_HOST_[A-Z_]*(SHARD|RESERVED_WALL_SECONDS):", text): + layout.add(number) + return layout + + +def shard_layout_changed(root: Path, paths: Iterable[str] | None, diff: str | None) -> bool: + """True when the diff changes how app-host unit suites are laid out over shards. + + A ci-macos.yml edit counts only when a hunk touches shard_layout_lines(), + or when its hunks are missing and the edit cannot be placed. An unreadable + file list returns False because the caller already runs every unit suite. + """ + if paths is None: + return False + stripped = {path.strip() for path in paths} + if stripped & set(SHARD_LAYOUT_PATHS[1:]): + return True + if MACOS_WORKFLOW_PATH not in stripped: + return False + hunks = changed_lines(diff).get(MACOS_WORKFLOW_PATH) if diff else None + if not hunks: + return True + try: + workflow = (root / MACOS_WORKFLOW_PATH).read_text(encoding="utf-8") + except (OSError, UnicodeError): + return True + return bool(hunks & shard_layout_lines(workflow)) + + def runs_in_admission( root: Path, paths: Iterable[str] | None, @@ -497,11 +556,13 @@ def main(argv: list[str]) -> int: 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) + layout = shard_layout_changed(args.root, paths, diff) + unit = layout or 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 ()} + # explicit requests for every suite, and a shard layout change needs every + # suite in its new order. + asked_for_every_suite = full or layout 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] = [] From 5007ac983a280dd1d70563e80b082af178a59ea2 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 25 Sep 2026 07:50:48 -0400 Subject: [PATCH 3/3] ci: count a removed or renamed shard setting as a layout change changed_lines() reports new-side lines only, so a ci-macos.yml edit that deleted a CMUX_APP_HOST_*_SHARD setting or a matrix shard entry, or renamed it to another key, left no layout line on the new side and took the one-suite canary. Scan the workflow's removed lines for those settings too. Co-Authored-By: Claude Opus 5.5 --- scripts/ci/choose_ci_suite.py | 30 +++++++++++++++++++++++++++++- tests/test_ci_change_areas.py | 10 ++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/scripts/ci/choose_ci_suite.py b/scripts/ci/choose_ci_suite.py index 23ee6e747efc..6551e161613c 100755 --- a/scripts/ci/choose_ci_suite.py +++ b/scripts/ci/choose_ci_suite.py @@ -375,6 +375,10 @@ def consumer_canary_selectors( return [] +SHARD_LAYOUT_SETTING_RE = re.compile(r"^ CMUX_APP_HOST_[A-Z_]*(SHARD|RESERVED_WALL_SECONDS):") +SHARD_MATRIX_ENTRY_RE = re.compile(r'^\s*\{"shard":') + + def shard_layout_lines(workflow: str) -> set[int]: """1-based lines of `app-host unit tests` that lay out its shards. @@ -391,11 +395,33 @@ def shard_layout_lines(workflow: str) -> set[int]: text = lines[number - 1] if re.match(r"^ [A-Za-z_-]+:", text): in_strategy = text.startswith(" strategy:") - if in_strategy or re.match(r"^ CMUX_APP_HOST_[A-Z_]*(SHARD|RESERVED_WALL_SECONDS):", text): + if in_strategy or SHARD_LAYOUT_SETTING_RE.match(text): layout.add(number) return layout +def removed_shard_layout_setting(diff: str) -> bool: + """True when a ci-macos.yml hunk removes a line that set the shard layout. + + changed_lines() reports new-side lines only, so a shard setting that an + edit deletes or renames to another key would not show up in + shard_layout_lines() of the new workflow. + """ + 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 + continue + if line.startswith("--- "): + continue + if path == MACOS_WORKFLOW_PATH and line.startswith("-"): + removed = line[1:] + if SHARD_LAYOUT_SETTING_RE.match(removed) or SHARD_MATRIX_ENTRY_RE.match(removed): + return True + return False + + def shard_layout_changed(root: Path, paths: Iterable[str] | None, diff: str | None) -> bool: """True when the diff changes how app-host unit suites are laid out over shards. @@ -413,6 +439,8 @@ def shard_layout_changed(root: Path, paths: Iterable[str] | None, diff: str | No hunks = changed_lines(diff).get(MACOS_WORKFLOW_PATH) if diff else None if not hunks: return True + if removed_shard_layout_setting(diff): + return True try: workflow = (root / MACOS_WORKFLOW_PATH).read_text(encoding="utf-8") except (OSError, UnicodeError): diff --git a/tests/test_ci_change_areas.py b/tests/test_ci_change_areas.py index eaabd33d09ef..b1aafff62b2d 100755 --- a/tests/test_ci_change_areas.py +++ b/tests/test_ci_change_areas.py @@ -5156,6 +5156,16 @@ def hunk(line: int, count: int = 1) -> str: assert not shard_layout_changed(ROOT, [workflow_path], hunk(compile_line)) # An edit nothing can place counts. assert shard_layout_changed(ROOT, [workflow_path], None) + # A shard setting deleted or renamed to another key counts, although the + # new-side line it leaves behind is not a layout line. + timeout_line = line_of(" CMUX_UNIT_TEST_TIMEOUT_SECONDS:") + for removed in (' CMUX_APP_HOST_GLOBAL_SEARCH_SHARD: "3"', ' {"shard": 7},'): + renamed = (f"--- a/{workflow_path}\n+++ b/{workflow_path}\n@@ -{timeout_line},1 +{timeout_line},1 @@\n" + f"-{removed}\n+ CMUX_APP_HOST_RENAMED: \"3\"\n") + assert shard_layout_changed(ROOT, [workflow_path], renamed), removed + unrelated = (f"--- a/{workflow_path}\n+++ b/{workflow_path}\n@@ -{timeout_line},1 +{timeout_line},1 @@\n" + "- CMUX_UNIT_TEST_TIMEOUT_SECONDS: \"1\"\n+ CMUX_UNIT_TEST_TIMEOUT_SECONDS: \"2\"\n") + assert not shard_layout_changed(ROOT, [workflow_path], unrelated) script = ROOT / "scripts/ci/choose_ci_suite.py" with tempfile.TemporaryDirectory() as directory: