Repository navigation
tests: stop three fixtures from killing the test host on window close - #8701
Merged
austinywang merged 1 commit intoAug 4, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThree test window creation paths now disable AppKit’s automatic release-on-close behavior, preventing test-host over-release during cleanup. ChangesTest Window Lifecycle
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
This was referenced Jul 23, 2026
Open
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Jul 25, 2026
Seven test call sites constructed a bare SavingTextView(). The product never does: makeFilePreviewTextView() builds an explicit TextKit 1 stack, because a default NSTextView is TextKit 2 and its selection path pegged the main thread on large documents (manaflow-ai#4576, manaflow-ai#5255). A bare init therefore has no configured text container, and it also skips applyFilePreviewTextEditorInsets(), which the factory applies. So these tests exercised a view the app refuses to ship, and failed on it: the save-shortcut tests read back an empty string instead of the saved text, and the inset test read nil where it expected a value. The files already disagreed with themselves. FilePreviewReviewFeedbackTests used the bare init at line 44 and the factory at line 408, CanvasShortcutContextTests uses the factory at three sites, and FilePreviewTextEditorTextKitTests exists to assert the factory yields a pure TextKit 1 view. FilePreviewPanelTextSavingTests 27 tests, 3 failures -> 0 failures FilePreviewReviewFeedbackTests 17 tests, 1 failure -> 0 failures Both suites need the window-release guard in manaflow-ai#8701 to reach these assertions at all; without it they crash the test host first. Both arms above were measured with that guard applied.
ejc3
force-pushed
the
fix-three-fixtures-window-release
branch
from
July 25, 2026 04:56
36e5918 to
7edaab5
Compare
ejc3
marked this pull request as ready for review
July 25, 2026 04:58
Contributor
Greptile SummaryThis PR fixes three test fixtures that were crashing the XCTest host process by closing an
Confidence Score: 5/5Safe to merge — all three changes are confined to the test target, apply the standard AppKit isReleasedWhenClosed = false guard that the rest of the codebase already uses, and carry no risk of regression in production code. The changes are minimal, test-only, and target an objectively wrong default (isReleasedWhenClosed = true under ARC) that was causing deterministic host crashes. The fix is consistent with how every production window in Sources/ handles the same situation. No new logic, no new state, no risk of behavioral change outside the three affected test suites. Files Needing Attention: No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as XCTest Function
participant AppKit as AppKit (NSWindow)
participant ARC as Swift ARC
Note over Test,ARC: Before fix (isReleasedWhenClosed = true, default)
Test->>AppKit: NSWindow(...)
AppKit-->>ARC: "ARC retains (refcount = 1)"
Test->>AppKit: window.close()
AppKit->>ARC: "AppKit releases (refcount = 0 → dealloc)"
Test-->>ARC: "local var goes out of scope → release (refcount = -1 💥)"
Note over Test,ARC: Over-release → host process crash
Note over Test,ARC: After fix (isReleasedWhenClosed = false)
Test->>AppKit: NSWindow(...)
AppKit-->>ARC: "ARC retains (refcount = 1)"
Test->>AppKit: "window.isReleasedWhenClosed = false"
Test->>AppKit: window.close()
Note over AppKit: AppKit skips extra release
Test-->>ARC: "local var goes out of scope → release (refcount = 0 → dealloc ✅)"
Reviews (1): Last reviewed commit: "tests: stop three fixtures from killing ..." | Re-trigger Greptile |
Three suites created an NSWindow and later closed it without opting out of AppKit's close-time release, so each close over-released a window ARC still held and took the whole test host down with it. A dead host is worse than a failing test: the suite reports no verdict, and every suite sharing that host loses its verdict too. TerminalNotificationSocketActionTests 2 restarts, 0 of 7 tests ran -> 0 restarts, 7 pass FilePreviewPanelTextSavingTests 2 restarts, 2 of 27 tests ran -> 0 restarts, 27 run FilePreviewReviewFeedbackTests 1 restart, 11 of 17 tests ran -> 0 restarts, 17 run FilePreviewPanelTextSavingTests closes a window in fourteen tests, all through one private windowHosting helper, so the guard goes there rather than at each call site. The other twenty-two suites in that file build windows and only ever orderOut them, which does not release, so they need nothing. Two of these suites still have assertion failures behind the crash that nobody could see while the host was dying: three in FilePreviewPanelTextSavingTests (24 of 27 pass) and one in FilePreviewReviewFeedbackTests (16 of 17 pass). Those are separate bugs and get their own change. The product already sets isReleasedWhenClosed = false everywhere it owns a window; only these fixtures were missing it.
ejc3
force-pushed
the
fix-three-fixtures-window-release
branch
from
July 31, 2026 06:55
7edaab5 to
4b04891
Compare
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. |
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Jul 31, 2026
Seven test call sites constructed a bare SavingTextView(). The product never does: makeFilePreviewTextView() builds an explicit TextKit 1 stack, because a default NSTextView is TextKit 2 and its selection path pegged the main thread on large documents (manaflow-ai#4576, manaflow-ai#5255). A bare init therefore has no configured text container, and it also skips applyFilePreviewTextEditorInsets(), which the factory applies. So these tests exercised a view the app refuses to ship, and failed on it: the save-shortcut tests read back an empty string instead of the saved text, and the inset test read nil where it expected a value. The files already disagreed with themselves. FilePreviewReviewFeedbackTests used the bare init at line 44 and the factory at line 408, CanvasShortcutContextTests uses the factory at three sites, and FilePreviewTextEditorTextKitTests exists to assert the factory yields a pure TextKit 1 view. FilePreviewPanelTextSavingTests 27 tests, 3 failures -> 0 failures FilePreviewReviewFeedbackTests 17 tests, 1 failure -> 0 failures Both suites need the window-release guard in manaflow-ai#8701 to reach these assertions at all; without it they crash the test host first. Both arms above were measured with that guard applied.
Contributor
|
Closeout review audit:
|
Contributor
|
Factual correction to the bot summaries: FilePreviewPanelTextSavingTests uses the shared windowHosting helper in two tests that create three windows total, not fourteen tests. The implementation boundary remains correct because every one of those three windows is closed through the paired helper. I corrected both occurrences in the PR body. |
austinywang
pushed a commit
that referenced
this pull request
Aug 4, 2026
Seven test call sites constructed a bare SavingTextView(). The product never does: makeFilePreviewTextView() builds an explicit TextKit 1 stack, because a default NSTextView is TextKit 2 and its selection path pegged the main thread on large documents (#4576, #5255). A bare init therefore has no configured text container, and it also skips applyFilePreviewTextEditorInsets(), which the factory applies. So these tests exercised a view the app refuses to ship, and failed on it: the save-shortcut tests read back an empty string instead of the saved text, and the inset test read nil where it expected a value. The files already disagreed with themselves. FilePreviewReviewFeedbackTests used the bare init at line 44 and the factory at line 408, CanvasShortcutContextTests uses the factory at three sites, and FilePreviewTextEditorTextKitTests exists to assert the factory yields a pure TextKit 1 view. FilePreviewPanelTextSavingTests 27 tests, 3 failures -> 0 failures FilePreviewReviewFeedbackTests 17 tests, 1 failure -> 0 failures Both suites need the window-release guard in #8701 to reach these assertions at all; without it they crash the test host first. Both arms above were measured with that guard applied. Co-authored-by: ejc3 <ejc3@users.noreply.github.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
2 participants
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.
Three suites built an
NSWindowand later closed it without opting out of AppKit's close-time release. ARC still holds a strong reference, so each close over-releases and takes the whole test host down.A dead host is worse than a failing test. The suite reports no verdict at all, and every suite sharing that host loses its verdict too, which is how a single unguarded window turns into a spray of unexplained reds elsewhere.
Measured, one suite per app host
Both arms ran on the same worktree with the same warm derived-data path, with only this change between them.
#8651is applied in both, since thecmuxTeststarget does not compile without it.TerminalNotificationSocketActionTests** TEST FAILED **** TEST SUCCEEDED **FilePreviewPanelTextSavingTests** TEST FAILED **FilePreviewReviewFeedbackTests** TEST FAILED **Where the guard goes, and why not everywhere
FilePreviewPanelTextSavingTestscreates three windows across two tests, and all three go through one privatewindowHostinghelper, so the guard belongs there rather than at three call sites. The other window fixtures declared inWindowAndDragTests.swiftbuild windows and only everorderOutthem, which does not release, so they need nothing. I mapped everyNSWindowsite in that file to its enclosing suite before touching it rather than applying the flag broadly.That mapping is the same rule that predicted these three suites in the first place: counting the tests that close a self-created unguarded window matches the measured host-restart count. Across the browser suites it lands at 16, 3, 1 and 1, and correctly predicts 0 for the one suite that fails assertions without ever restarting.
The product already sets
isReleasedWhenClosed = falseeverywhere it owns a window (BrowserPanel.swift,BrowserPrewarmedWebViewPool.swift,BrowserPopupWindowController.swift,ReleasingWindowController.swift). Only these fixtures were missing it, which is why nothing inSourceschanges here.What is still red behind the crash
Fixing the crash exposed assertions nobody could see while the host was dying. These are separate bugs and get their own change:
The two
SavingTextView…SaveShortcutfailures look like one mechanism: a convenience initializer that builds the view without a text container, so there is no text to save.Read the restart count and the test count when checking this, not the verdict line. A host that dies mid-run still prints a summary, and these logs also carry earlier
Executed 0 tests, with 0 failureslines that a tail-only grep would read as fine.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Prevents test host crashes when three test fixtures close their NSWindow. Sets
isReleasedWhenClosed = falseso closing doesn’t over-release under ARC, removing host restarts and letting all tests run.testWindow.isReleasedWhenClosed = false.windowHostinghelper used by two tests that create three windows.window.isReleasedWhenClosed = false.Written for commit 4b04891. Summary will update on new commits.
Summary by CodeRabbit