Split mega test file, bump CI timeout, stream xcodebuild output - #1717
Merged
Merged
Conversation
…o 30min CmuxWebViewKeyEquivalentTests.swift grew to 15,907 lines with 100+ test classes. Swift compiles per-file, so this single file serialized all type-checking onto one compiler process, pushing CI past the 20-minute timeout after core-file changes. Split into 10 domain-based files (1k-3k lines each) so Xcode can compile them in parallel. Also bump timeout-minutes from 20 to 30 for headroom, stream xcodebuild output via tee instead of capturing to a variable (makes CI logs debuggable), and add 5 test files that were missing from the pbxproj Sources build phase.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR adds extensive unit test coverage across multiple cmux subsystems including notifications, omnibar, shortcuts, sidebar, tab manager, terminal/ghostty, window drag handling, and workspace functionality. It also adjusts CI workflow timeouts and test output capture patterns, and removes one test file from the project build. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/BrowserConfigTests.swift">
<violation number="1" location="cmuxTests/BrowserConfigTests.swift:1">
P3: Replace fixed RunLoop delay waits with condition-based expectations to reduce CI flakiness and avoid unnecessary test latency.</violation>
</file>
<file name="cmuxTests/TerminalAndGhosttyTests.swift">
<violation number="1" location="cmuxTests/TerminalAndGhosttyTests.swift:2338">
P2: Two tests assert opposite outcomes for the exact same input, so at least one expectation is wrong and the suite becomes unreliable.</violation>
</file>
<file name="GhosttyTabs.xcodeproj/project.pbxproj">
<violation number="1" location="GhosttyTabs.xcodeproj/project.pbxproj:519">
P1: Newly added split test files are not wired into the `cmuxTests` Sources build phase, so they won’t run in CI.</violation>
</file>
<file name="cmuxTests/TabManagerUnitTests.swift">
<violation number="1" location="cmuxTests/TabManagerUnitTests.swift:23">
P2: `drainMainQueue()` ignores the `XCTWaiter` result, which can hide timeout failures and make tests pass with stale async state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| FA000001A1B2C3D4E5F60718 /* WorkspaceStressProfileTests.swift */, | ||
| A5008380 /* BrowserFindJavaScriptTests.swift */, | ||
| A5008382 /* CommandPaletteSearchEngineTests.swift */, | ||
| 970226F3C99D0D937CD00539 /* BrowserConfigTests.swift */, |
There was a problem hiding this comment.
P1: Newly added split test files are not wired into the cmuxTests Sources build phase, so they won’t run in CI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At GhosttyTabs.xcodeproj/project.pbxproj, line 519:
<comment>Newly added split test files are not wired into the `cmuxTests` Sources build phase, so they won’t run in CI.</comment>
<file context>
@@ -489,6 +516,21 @@
FA000001A1B2C3D4E5F60718 /* WorkspaceStressProfileTests.swift */,
A5008380 /* BrowserFindJavaScriptTests.swift */,
A5008382 /* CommandPaletteSearchEngineTests.swift */,
+ 970226F3C99D0D937CD00539 /* BrowserConfigTests.swift */,
+ 58C7B1B978620BE162CC057E /* BrowserPanelTests.swift */,
+ 02FC74F2C27127CC565B3E8C /* TerminalAndGhosttyTests.swift */,
</file context>
|
|
||
|
|
||
| final class GhosttyTerminalViewVisibilityPolicyTests: XCTestCase { | ||
| func testImmediateStateUpdateAllowedWhenHostNotInWindow() { |
There was a problem hiding this comment.
P2: Two tests assert opposite outcomes for the exact same input, so at least one expectation is wrong and the suite becomes unreliable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/TerminalAndGhosttyTests.swift, line 2338:
<comment>Two tests assert opposite outcomes for the exact same input, so at least one expectation is wrong and the suite becomes unreliable.</comment>
<file context>
@@ -0,0 +1,2578 @@
+
+
+final class GhosttyTerminalViewVisibilityPolicyTests: XCTestCase {
+ func testImmediateStateUpdateAllowedWhenHostNotInWindow() {
+ XCTAssertTrue(
+ GhosttyTerminalView.shouldApplyImmediateHostedStateUpdate(
</file context>
| DispatchQueue.main.async { | ||
| expectation.fulfill() | ||
| } | ||
| XCTWaiter().wait(for: [expectation], timeout: 1.0) |
There was a problem hiding this comment.
P2: drainMainQueue() ignores the XCTWaiter result, which can hide timeout failures and make tests pass with stale async state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/TabManagerUnitTests.swift, line 23:
<comment>`drainMainQueue()` ignores the `XCTWaiter` result, which can hide timeout failures and make tests pass with stale async state.</comment>
<file context>
@@ -0,0 +1,976 @@
+ DispatchQueue.main.async {
+ expectation.fulfill()
+ }
+ XCTWaiter().wait(for: [expectation], timeout: 1.0)
+}
+
</file context>
| @@ -0,0 +1,3108 @@ | |||
| import XCTest | |||
There was a problem hiding this comment.
P3: Replace fixed RunLoop delay waits with condition-based expectations to reduce CI flakiness and avoid unnecessary test latency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/BrowserConfigTests.swift:
<comment>Replace fixed RunLoop delay waits with condition-based expectations to reduce CI flakiness and avoid unnecessary test latency.</comment>
lawrencecchen
added a commit
that referenced
this pull request
Mar 18, 2026
…meout The previous PR (#1717) added 15 test files to the pbxproj PBXBuildFile and PBXGroup sections but missed adding them to the cmuxTests Sources build phase (F1000005), so they were never compiled in CI. Also add executionTimeAllowance = 30s to AppDelegateShortcutRoutingTests to prevent testCmdWClosesWindowWhenClosingLastSurfaceInLastWorkspace from hanging indefinitely on CI (the actual root cause of the timeout).
2 tasks
lawrencecchen
added a commit
that referenced
this pull request
Mar 18, 2026
The previous PR (#1717) added 15 test files to the pbxproj PBXBuildFile and PBXGroup sections but missed adding them to the cmuxTests Sources build phase (F1000005), so they were never compiled in CI. Also add executionTimeAllowance = 30s to AppDelegateShortcutRoutingTests to prevent testCmdWClosesWindowWhenClosingLastSurfaceInLastWorkspace from hanging indefinitely on CI (the actual root cause of the timeout). Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
lawrencecchen
added a commit
that referenced
this pull request
Mar 18, 2026
The test split PR (#1717) applied output streaming (tee) and timeout bump only to ci-macos-compat.yml, not ci.yml. The main CI tests job was still capturing all xcodebuild output in a $() subshell (making logs blank) and using a 20 minute timeout (too tight after the test file split). Port the same fixes from ci-macos-compat.yml: - Stream xcodebuild output via tee so CI logs show progress in real time - Bump timeout-minutes from 20 to 30 - Update the SPM retry guard test for the new tee pattern
2 tasks
lawrencecchen
added a commit
that referenced
this pull request
Mar 19, 2026
* Fix CI test timeout: stream xcodebuild output and bump timeout to 30m The test split PR (#1717) applied output streaming (tee) and timeout bump only to ci-macos-compat.yml, not ci.yml. The main CI tests job was still capturing all xcodebuild output in a $() subshell (making logs blank) and using a 20 minute timeout (too tight after the test file split). Port the same fixes from ci-macos-compat.yml: - Stream xcodebuild output via tee so CI logs show progress in real time - Bump timeout-minutes from 20 to 30 - Update the SPM retry guard test for the new tee pattern * Fix hanging test: auto-confirm window close in last-surface Cmd+W test testCmdWClosesWindowWhenClosingLastSurfaceInLastWorkspace hung for 26+ minutes on CI because it sent Cmd+W to close the last surface without setting debugCloseMainWindowConfirmationHandler. The window close path shows a modal confirmation dialog that blocks the RunLoop indefinitely on headless runners. Set the handler to auto-confirm, matching the pattern used by testCmdCtrlWClosesWindowAfterConfirmation. * Skip last-surface close test on CI: PTY teardown blocks on headless runners The confirmation handler fix wasn't sufficient. The hang is in Ghostty surface/PTY teardown when closing the last terminal surface, not the window close confirmation. Shell process termination blocks indefinitely on headless CI runners without a TTY. Skip with XCTSkip when CI env var is set. The test still runs locally and can be covered via E2E on runners with virtual displays. * Skip hanging test via -skip-testing flag in xcodebuild The CI env var isn't visible inside xcodebuild's test host process, so the XCTSkip approach didn't work. Use -skip-testing on the xcodebuild command line instead. --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This was referenced Mar 19, 2026
This was referenced Mar 27, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…put (manaflow-ai#1717) CmuxWebViewKeyEquivalentTests.swift grew to 15,907 lines with 100+ test classes. Swift compiles per-file, so this single file serialized all type-checking onto one compiler process, pushing CI past the 20-minute timeout after core-file changes. Split into 10 domain-based files (1k-3k lines each) so Xcode can compile them in parallel. Also bump timeout-minutes from 20 to 30 for headroom, stream xcodebuild output via tee instead of capturing to a variable (makes CI logs debuggable), and add 5 test files that were missing from the pbxproj Sources build phase. Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…#1743) The previous PR (manaflow-ai#1717) added 15 test files to the pbxproj PBXBuildFile and PBXGroup sections but missed adding them to the cmuxTests Sources build phase (F1000005), so they were never compiled in CI. Also add executionTimeAllowance = 30s to AppDelegateShortcutRoutingTests to prevent testCmdWClosesWindowWhenClosingLastSurfaceInLastWorkspace from hanging indefinitely on CI (the actual root cause of the timeout). Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Fix CI test timeout: stream xcodebuild output and bump timeout to 30m The test split PR (manaflow-ai#1717) applied output streaming (tee) and timeout bump only to ci-macos-compat.yml, not ci.yml. The main CI tests job was still capturing all xcodebuild output in a $() subshell (making logs blank) and using a 20 minute timeout (too tight after the test file split). Port the same fixes from ci-macos-compat.yml: - Stream xcodebuild output via tee so CI logs show progress in real time - Bump timeout-minutes from 20 to 30 - Update the SPM retry guard test for the new tee pattern * Fix hanging test: auto-confirm window close in last-surface Cmd+W test testCmdWClosesWindowWhenClosingLastSurfaceInLastWorkspace hung for 26+ minutes on CI because it sent Cmd+W to close the last surface without setting debugCloseMainWindowConfirmationHandler. The window close path shows a modal confirmation dialog that blocks the RunLoop indefinitely on headless runners. Set the handler to auto-confirm, matching the pattern used by testCmdCtrlWClosesWindowAfterConfirmation. * Skip last-surface close test on CI: PTY teardown blocks on headless runners The confirmation handler fix wasn't sufficient. The hang is in Ghostty surface/PTY teardown when closing the last terminal surface, not the window close confirmation. Shell process termination blocks indefinitely on headless CI runners without a TTY. Skip with XCTSkip when CI env var is set. The test still runs locally and can be covered via E2E on runners with virtual displays. * Skip hanging test via -skip-testing flag in xcodebuild The CI env var isn't visible inside xcodebuild's test host process, so the XCTSkip approach didn't work. Use -skip-testing on the xcodebuild command line instead. --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This was referenced Apr 9, 2026
This was referenced Apr 11, 2026
This was referenced Apr 18, 2026
This was referenced Apr 27, 2026
israeligal
pushed a commit
to israeligal/cmux
that referenced
this pull request
May 2, 2026
…meout The previous PR (manaflow-ai#1717) added 15 test files to the pbxproj PBXBuildFile and PBXGroup sections but missed adding them to the cmuxTests Sources build phase (F1000005), so they were never compiled in CI. Also add executionTimeAllowance = 30s to AppDelegateShortcutRoutingTests to prevent testCmdWClosesWindowWhenClosingLastSurfaceInLastWorkspace from hanging indefinitely on CI (the actual root cause of the timeout).
This was referenced May 4, 2026
This was referenced May 12, 2026
rodchristiansen
pushed a commit
to rodchristiansen/cmux
that referenced
this pull request
Sep 2, 2026
…meout The previous PR (manaflow-ai#1717) added 15 test files to the pbxproj PBXBuildFile and PBXGroup sections but missed adding them to the cmuxTests Sources build phase (F1000005), so they were never compiled in CI. Also add executionTimeAllowance = 30s to AppDelegateShortcutRoutingTests to prevent testCmdWClosesWindowWhenClosingLastSurfaceInLastWorkspace from hanging indefinitely on CI (the actual root cause of the timeout).
This branch was successfully deployed
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
1 participant
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.
Summary
CmuxWebViewKeyEquivalentTests.swift(15,907 lines, 100+ test classes) into 10 domain-based files (1k-3k lines each). Swift compiles per-file, so this lets Xcode parallelize type-checking across 10 compiler processes instead of serializing on one.ci-macos-compat.ymltimeout from 20 to 30 minutes. The old limit was tight even for incremental builds after core-file changes.xcodebuild testoutput viateeinstead of capturing to$OUTPUT. CI logs now show real-time progress instead of 20 minutes of silence.cmuxTests/but missing from the pbxproj Sources build phase (never compiled).Fixes the timeout on https://github.com/manaflow-ai/cmux/actions/runs/23231286231
Test plan
ci-macos-compatpasses on this PR (compiles + runs all unit tests)Summary by cubic
Split a 16k-line test into 10 domain-based files to parallelize Swift type-checking and prevent CI timeouts. Increased the macOS compat job timeout to 30 minutes and stream
xcodebuildoutput for real-time logs. Also added five missing test files to the Xcode project so they compile.Refactors
CmuxWebViewKeyEquivalentTests.swiftwith 10 files (1k–3k lines each) to let Xcode compile in parallel.Bug Fixes
ci-macos-compat.ymltimeout from 20 to 30 minutes for headroom.xcodebuild testviateeto show live progress in CI logs.Written for commit e9fbcfe. Summary will update on new commits.
Summary by CodeRabbit
Tests
Chores