Repository navigation
tests: consolidate the 17 duplicate-name cmuxTests suites - #13786
Merged
teamleaderleo merged 2 commits intoSep 23, 2026
Merged
Conversation
|
Warning Review limit reachedNext included review available in 22 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 (14)
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 |
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
RFC manaflow-ai#13519 item 2. Seventeen cmuxTests suites share a name with an existing package test suite (227 app-host tests). Diffing them assertion by assertion shows five are not duplicates at all, so this consolidates only the twelve that are. Removed from the app-host bundle (80 tests), after porting every assertion the package copy did not already make: - SocketControlSettingsTests (38): the package suite covers mode resolution and the XCTest fallback; the app copy covered per-channel default paths and stable-socket hijack refusal (user-scoped alias, case variant, /private/tmp alias, leaf symlink, 64-deep chain) plus the pre-listener startup probe. 33 tests move to CmuxSettings as SocketControlSettingsChannelTests; 5 were already covered. - ShortcutWhenClauseTests (9), CommandPaletteEmojiTitleSearchTests (3), RecentlyClosedBrowserStackTests (3), WorkspaceRemoteDaemonManifestTests (2): strict subsets of the package suites, deleted outright. - RemoteLoopbackHTTPRequestRewriterTests (9), RovoDevHookConfigTests (6), SocketControlPasswordStoreTests (4), QuitConfirmationPolicyTests (4): the uncovered cases move into the package suite first. - DiffViewerPickerCommandRunnerTests: the two runner tests go; the WKURLSchemeTask teardown test stays, since it drives the app-target CmuxDiffViewerURLSchemeHandler. Left in place, with reasons: - NotificationFeedHistoryTests and SettingsSearchIndexTests are name collisions across module boundaries, not duplicates — the app copies test app-target types that merely share a name with unrelated package types. 0 of 41 assertions are covered elsewhere. - SidebarDropPlannerTests: the same-named package file mostly tests SidebarWorkspaceReorderDropResolver; crossWindowInsertion and targetIndex are asserted nowhere in Packages. - GhosttyCopyActionResolverTests is a deliberate app-target linkage guard. - CommandPaletteSearchEngineTests, CommandPaletteNucleoFFITests and SidebarWorkspaceAuxiliaryDetailVisibilityTests sit in files owned by open PRs and are left for a follow-up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
teamleaderleo
force-pushed
the
dupe-suite-consolidation
branch
from
September 22, 2026 23:29
0ef3945 to
e8e9277
Compare
teamleaderleo
enabled auto-merge (squash)
September 22, 2026 23:59
This was referenced Sep 23, 2026
teamleaderleo
added a commit
that referenced
this pull request
Sep 23, 2026
* ci: run package tests on the pull requests that change packages `macos / swift-package-tests` is gated on `inputs.full_suite == 'true'`, and `full_suite` is false for every pull request under `CI_PULL_REQUEST_SUITE=compile-only`. The job is therefore skipped on every PR. The only macOS signal a PR gets is `macOS compile admission`, which builds package *library* targets and never their test targets, so a change to a package's tests currently lands with zero CI execution of those assertions. #13786 and #13790 move ~150 of them into package test targets. Route the lane from changed paths, the way the `cli` lane selects itself (#13760). A new `swift_packages` change area asks `scripts/ci/select_package_tests.py` — the same selector the job already uses to narrow its package list — whether a diff can affect any package the job runs. Only paths that feed a package are offered to it: * The selector fails open, so the lane's own workflow and scripts select all 33 packages. On main that is right; on a pull request it is the 30-minute sweep under another name. Over the last 200 merged PRs, routing those would have queued 34 full runs. * A package outside the job's list selects nothing, so the lane would start, check out submodules and test zero packages. Measured over the last 200 merged pull requests: 28 would run the lane (0 do today), testing a median of 2 packages and never more than 15, never all 33 and never zero. The full suite is untouched: `app-host-unit-tests`, `tests-build-and-lag` and the Release jobs stay gated on `full_suite` alone, and a test in this change pins that set so a new lane cannot silently join it. The Release Ghostty CLI helper steps inside the job now also require `full_suite`, since only the `full_suite`-gated Release jobs consume that artifact. Also collect the operating system's crash report when a package test runner dies on a signal. `main` is currently red from a SIGBUS in CmuxSettingsUI, and all the job records is SwiftPM's one-line "Exited with unexpected signal code 10" — no faulting address, no frames. Replaying the retry guard against that job's captured output shows widening its signal list would not have retried it anyway: the runner had already emitted 401 lines of test output, which the guard's third clause correctly refuses to retry. The signal list is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: catch full package sweeps on mixed routed changes * ci: keep targeted package selection aligned with routing --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
RFC #13519 item 2: the 17
cmuxTestssuites that share a name with an existing package test suite (227 app-host tests). I diffed every pair assertion by assertion — same name turned out not to mean same coverage in five of the seventeen — and consolidated only where the app-host copy was genuinely redundant.Net: 80 tests leave the app-host bundle; 44 tests arrive in package targets; no assertion is lost.
Deleting a suite because a same-named package suite exists would have dropped ~50 real assertions here. The table below is explicit about which ones and why.
Disposition of all 17
SocketControlSettingsTestsCmuxSettingsasSocketControlSettingsChannelTests; 5 already covered by the existing package suite; app copy deletedShortcutWhenClauseTestsRemoteLoopbackHTTPRequestRewriterTestsCmuxRemoteWorkspace(Origin/Referer on alias subdomains, response subdomain rewrite, leading-dot subdomain cookie domain); app copy deletedRovoDevHookConfigTestsCMUXAgentLaunch; app copy deletedSocketControlPasswordStoreTestsverifywith nothing configured); app copy deletedQuitConfirmationPolicyTestsdirtyOnly, not just the default mode); app copy deletedCommandPaletteEmojiTitleSearchTestsRecentlyClosedBrowserStackTestsDiffViewerPickerCommandRunnerTestsCmuxDiffViewerURLSchemeHandler(app target) through a realWKURLSchemeTask, which is genuinely host-dependentWorkspaceRemoteDaemonManifestTestsCmuxCoreTests/…decodesFromInfoDictionary; the versioned cache path covered byCmuxRemoteWorkspaceTests/…cachePathShapewith identical inputsNotificationFeedHistoryTestsNotificationFeedHistoryStore/…Persistence(app target: retention trimming, snapshot quarantine/recovery, revision monotonicity, mobile-host RPC). The package copy testsNotificationFeedProjectioninCmuxMobileShellUI(iOS list rows, disclosure groups, grouping). 0 of 28 covered.SidebarDropPlannerTestsSidebarDropPlannerPackageTests, 9 of whose 11 tests exerciseSidebarWorkspaceReorderDropResolver.crossWindowInsertionandtargetIndexare called nowhere in any package test. 0 of 18 covered. It is class-(a) movable, so it belongs to the suite-relocation stream, not here.SettingsSearchIndexTestsSources/SettingsSearchIndex.swiftdeclares an app-ownedenum SettingsSearchIndexwith a static API and its own alias table;CmuxSettingsUIdeclares an unrelatedpublic struct SettingsSearchIndexwith an instance API. Two production types, same name.GhosttyCopyActionResolverTestsCommandPaletteSearchEngineTestscmuxTests/CommandPaletteSearchEngineTests.swift)CommandPaletteNucleoFFITestscmuxTests/CommandPaletteNucleoFFITests.swift)SidebarWorkspaceAuxiliaryDetailVisibilityTestscmuxTests/WorkspaceUnitTests.swift)Why the big one was worth porting rather than deleting
SocketControlSettingsTestsis the clearest case of "same name, complementary coverage". The package suite covers mode resolution, managed-device forcing, and the XCTest socket fallback. The app-host copy covered a disjoint area: per-channel default paths (stable/nightly/staging/tagged-debug), refusal to hijack the stable socket through a user-scoped alias, a case variant, a/private/tmpalias, a leaf symlink or a 64-deep symlink chain, and the pre-listener startup probe. Every symbol it touches isCmuxSettingsor Foundation — the app host bought it nothing.Verification
swift teston each touched package (CmuxSettings,CMUXAgentLaunch,CmuxRemoteWorkspace,CmuxBrowser).python3 scripts/normalize-pbxproj.pyafter unwiring the four deleted files.scripts/lint-pbxproj-test-wiring.sh— ok (1047 test files).tests/test_ci_test_execution_registry.py,tests/test_normalize_pbxproj.py,tests/test_sync_test_wiring.py— all pass. (tests/test-execution.tomlregisters Python/shell regressions only, so no registry change was needed.)🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Consolidates the 17 app-host
cmuxTestssuites that share a name with package test suites, diffing each pair assertion-by-assertion first: five "same-name" pairs turned out not to be duplicates, so only twelve were consolidated. Net: 80 tests leave the app-host bundle, 44 land in package targets, and no assertion is lost.Disposition
ShortcutWhenClauseTests,CommandPaletteEmojiTitleSearchTests,RecentlyClosedBrowserStackTests,WorkspaceRemoteDaemonManifestTests.CMUXAgentLaunch(RovoDevHookConfigTests),CmuxRemoteWorkspace(RemoteLoopbackHTTPRequestRewriterTests), andCmuxSettings(SocketControlPasswordStoreTests,QuitConfirmationPolicyTests), then deleted the app copies.SocketControlSettingsTestsintoCmuxSettingsasSocketControlSettingsChannelTests(per-channel defaults, socket hijack refusal, pre-listener startup probe); the other 5 were already covered.WKURLSchemeTaskteardown test forDiffViewerPickerCommandRunnerTests; it drives the app-targetCmuxDiffViewerURLSchemeHandler, and the runner tests moved toCmuxBrowser(with the concurrency-ceiling and cancellation assertions ported into the package fixture).Left alone
NotificationFeedHistoryTestsandSettingsSearchIndexTestsare name collisions across module boundaries, not duplicates: the paired tests exercise unrelated production types, so 0 of their 41 assertions are covered elsewhere.SidebarDropPlannerTeststests app-side types (crossWindowInsertion,targetIndex) that no package test exercises.GhosttyCopyActionResolverTestsis a deliberate app-target linkage guard.CommandPaletteSearchEngineTests,CommandPaletteNucleoFFITests, andSidebarWorkspaceAuxiliaryDetailVisibilityTestssit in files owned by in-flight PRs and are deferred.Written for commit 760c943. Summary will update on new commits.