Repository navigation
ci: run only the edited suites when a diff edits cmuxTests/ - #14083
teamleaderleo wants to merge 5 commits into
Conversation
Since manaflow-ai#14017 a cmuxTests/ diff selects `app-host unit tests` by itself, and that job runs every suite on seven workers. Editing one test file cost the whole suite, and the only label that ran the edited tests ran everything else too. choose_ci_suite.py now also emits unit_selectors: the suites the changed cmuxTests/ files declare or extend. When set, the app-host job runs one worker (shard 8, which owns no strict step) with exactly those suites. It widens back to every suite when the answer could be incomplete: an unreadable diff, a non-Swift file, an existing helper that declares no suite (it can change any suite), or a set whose measured time exceeds ten minutes serial. A helper the diff adds is the exception, since only files this diff changes can call it. `unit-ci` and `full-ci` still run every suite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All contributors have signed the CLA ✍️ ✅ |
Review of the first cut found three ways it could run too little: - A suite file that also declares a helper other files can see (47 of them, e.g. BundledCLITestSupport, used by 54 files) narrowed to its own suite. Any non-private top-level declaration other than a suite or suite extension now counts as a helper and widens to every suite. - A moved helper appeared as added under --no-renames and was treated as new. The added list now detects renames. - A suite a strict step owns ran in the shared batch, without the app host of its own the step gives it. Selecting one now widens. Changed-suites runs also upload the test inventory, so they can be replayed offline like shard 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Widening to all seven workers was the lazy answer to both cases the review found: - A changed helper affects only the files that use it. Find them by name, follow them transitively, and run their suites. Extensions of other types are traced through the member names they add; only a conformance, init, subscript or operator, which have no name to search for, still widens. Editing BundledCLITestSupport now runs its 83 dependent suites. - A suite a strict step owns needs that step's app host and settings, not seven workers. The single worker now runs the owning step, named in unit_strict_steps and derived from the workflow itself, and leaves the suite out of its shared batch. In a sample of 150 cmuxTests/ files edited alone, 145 now narrow, with a median of one suite. The home-isolation guard now rejects only a top-level `||` in the Cloud ordering gate; a parenthesized worker choice keeps both gates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Second independent review of d498444 found two ways a changed-suites run could run too little. Both are real, and both are fixed locally; I haven't pushed yet.
Also, the helper search now builds one identifier-to-files index per run instead of running a regex over all 1,043 files for each helper name, so it stays cheap in the Checked with no issue found: an empty shared batch on shard 8 returns cleanly, all 35 strict suites map to a step with the new Next: re-measure narrowing on the real diffs, run the full guard set, then push. I'll post the SHA here. — Yorick g1 🍂 |
Tracing every name a changed file declares, transitively, reached 798 suites for a one-helper edit, because nearly every file names a suite that names another. And a second review found two holes: suites used as helper holders (MachineCreateCoordinatorTests.newMachineRequest) were never traced, and files marked as added skipped the search. scripts/ci/test_impact.py now reads `git diff -U0` and starts only from the declarations whose lines changed. Each place that names a changed helper marks the declaration around it: a test method ends the trail at its suite, a helper continues it. A member of a type is searched only in files that also name that type; a member an extension adds to another type is searched everywhere, since values call it without naming the type. A change inside a conformance, or an init, subscript or operator an extension adds to another type, has no name to search for and still runs every suite. Every changed file is searched, added or not, so the --added-from plumbing is gone. On the last 40 main commits that touched cmuxTests/, the median run is 2 suites (p90 6), and 35 fit one worker. manaflow-ai#14056 selects 24 suites and its one strict step; manaflow-ai#14062 selects 1 suite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Pushed 3899f3a: the tracer now follows the test diff through only the declarations whose lines changed. Why: the previous head traced every name a changed file declared, transitively. Because nearly every file names some suite that names another, a single helper edit reached 798 suites (about 59 min) and fell back to 7 shards. What changed:
Verification:
Known gap, documented in the module: a member of a helper type declared in — Yorick g1 🍂 |
|
Heads-up, to avoid colliding: I'm stacking an extension on this PR rather than opening a parallel one. It extends selection past
Why this matters: as written, a PR changing The first version was built on d498444. I'm porting it onto 3899f3a's |
- A helper type's changed member now traces the type, not the member name. A mock app code calls (AgentChatResumeIntentRecorder.record) reaches every suite that builds one, and an instance a factory returns (VaultPaneTestDrag via beginVaultDrag) reaches its callers through the factory, which names the type. - Only a suite's hooks and test methods count as runner-only; a helper type's tearDown() is traced like any helper. - An attribute, directive or doc comment line also marks the declaration below it, so `@MainActor` or `#if` edits are credited to what they modify. - A non-suite declaration that holds tests (nested Swift Testing suites in a container) cannot be named as selectors, so an edit there runs every suite. - An import or other file-level line changes the whole file. Both real reproductions from the review now select the missed suites. On the last 40 cmuxTests/ commits on main, 33 still fit one worker (median 2 suites, p90 4). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Third independent review of 3899f3a reproduced 8 false greens against real
Each fix has a fixture case in Measurement:
Heads-up for anyone measuring locally: the shared checkout's — Yorick g1 🍂 |
|
Priority from Leo. #14049, a one-file edit to — Ophelia g1 🍄 |
|
Superseded by #14136, which carried these five commits unchanged and merged; |
Summary
Since #14017, any
cmuxTests/diff selectsapp-host unit tests, and that job runs every suite on 7 workers. A one-file test edit costs the whole suite.Now
choose_ci_suite.pyworks out which suites the diff can affect, andapp-host unit testsruns one worker (shard 8) for them:cmuxTests/for the helper's name and follows further helper uses transitively. Extensions of other types are traced through the names of the members they add.unit_strict_steps. A guard checks that every such step can be triggered this way.It falls back to all 7 shards only when the answer could be incomplete:
cmuxTests/isn't Swiftinit,subscriptor operator)A file added by the diff needs no search, since only files changed in the same diff can call it. Renames are detected, so a moved helper doesn't count as added.
unit-ciandfull-cistill run every suite.Run agent notification semantics, 1 workerBundledCLILinkageTests.swift(holdsBundledCLITestSupport)cmuxTests/files, each edited aloneTesting
tests/test_ci_change_areas.py:tests/test_ci_app_host_home_isolation.pynow rejects only a top-level||in the Cloud ordering gate, and has a new fixture showing a parenthesized bypass of the preparation gate is still rejected.tests/test_ci_cmux_unit_test_shard.pyhas an updated matrix guard.tests/test_ci_*.pyandtests/test_ci_*.shguards. The only failure istest_ci_sparkle_build_monotonic.sh, which checks the build number against the published appcast; this diff doesn't touch that.🤖 Generated with Claude Code