Repository navigation
Fix flaky FileExplorerStoreTests (macOS Compatibility) - #2880
Conversation
Tests used fixed Task.sleep(100ms) waits to let unstructured Tasks started by setRootPath/expand/setProvider hop to @mainactor and mutate @published state. On warp-macos-15 the 100ms budget wasn't enough, so testExpandedNodesSurviveStoreRecreation hit a force-unwrap nil crash. Replace the sleeps with polling waits on observable state (rootNodes, children, error, isRootLoading) and pin the test class to @mainactor so reads and writes share an actor.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/FileExplorerStoreTests.swift`:
- Around line 153-154: The test can pass before the initial load runs because it
only waits for store.isRootLoading == false; change the test to first wait for
the load to start and then for it to finish (e.g. await waitFor("{start}") {
store.isRootLoading == true } followed by await waitFor("{finish}") {
store.isRootLoading == false }) so the unstructured root-load task has actually
been executed; locate usages of waitFor and the store.isRootLoading check in
FileExplorerStoreTests.swift and update the assertion sequence around the
initial root load to assert start->finish rather than only finish.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8462ee1-6ecc-44b7-a866-2f4b51240b8b
📒 Files selected for processing (1)
cmuxTests/FileExplorerStoreTests.swift
Greptile SummaryThis PR fixes flaky Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as @MainActor Test
participant Store as FileExplorerStore
participant Task as Unstructured Task
participant Provider as MockFileExplorerProvider
Test->>Store: setRootPath("/home/user/project")
Store->>Store: "reload() → isRootLoading = true"
Store->>Task: "launch Task { loadChildren }"
Test->>Test: "waitFor("root loaded") { condition }"
loop Poll every 10ms (main actor yields)
Test->>Test: condition() → false
Test->>Test: await Task.sleep(10ms) [yields main actor]
Task-->>Store: "@MainActor loadChildren runs"
Store->>Provider: listDirectory(path:)
Provider-->>Store: [FileExplorerEntry]
Store->>Store: "rootNodes = children"
Store->>Store: "isRootLoading = false"
Test->>Test: condition() → true ✓
end
Test->>Test: assertions on rootNodes
Reviews (1): Last reviewed commit: "Fix flaky FileExplorerStoreTests" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| store.collapse(node: srcNode) | ||
| srcNode.children = nil |
There was a problem hiding this comment.
children = nil assignment
After a failed expand, loadChildren's catch block only sets parentNode.isLoading and parentNode.error — it never assigns parentNode.children — so srcNode.children is already nil when this line runs. The explicit nil-out is a no-op and slightly misleads the reader into thinking the error path might leave children non-nil. Either remove it, or add a comment explaining it's a defensive guard against a potential future refactor.
| store.collapse(node: srcNode) | |
| srcNode.children = nil | |
| store.collapse(node: srcNode) | |
| store.expand(node: srcNode) |
There was a problem hiding this comment.
Fixed in 03dc505 — dropped the redundant srcNode.children = nil. The error path in loadChildren never assigns children, so it was a no-op.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
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/FileExplorerStoreTests.swift">
<violation number="1" location="cmuxTests/FileExplorerStoreTests.swift:66">
P2: `waitFor` times out with `XCTFail` and `return`, but does not abort the test, so callers can continue into force-unwrap/index access and crash after a failed wait.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- waitFor now throws on timeout instead of XCTFail + return, so callers abort via `try await` and don't fall through to force-unwraps (cubic, cmuxTests/FileExplorerStoreTests.swift:66) - In testExpandedRemoteNodesHydrateWhenProviderBecomesAvailable, wait for the provider to actually be consulted, not just for isRootLoading == false which can be true before the unstructured load Task has started (CodeRabbit, line 154) - Drop redundant srcNode.children = nil in testStaleErrorClearsOnRetry; the error path in loadChildren never assigns children (Greptile, line 244)
) * Fix flaky FileExplorerStoreTests Tests used fixed Task.sleep(100ms) waits to let unstructured Tasks started by setRootPath/expand/setProvider hop to @mainactor and mutate @published state. On warp-macos-15 the 100ms budget wasn't enough, so testExpandedNodesSurviveStoreRecreation hit a force-unwrap nil crash. Replace the sleeps with polling waits on observable state (rootNodes, children, error, isRootLoading) and pin the test class to @mainactor so reads and writes share an actor. * Address review feedback - waitFor now throws on timeout instead of XCTFail + return, so callers abort via `try await` and don't fall through to force-unwraps (cubic, cmuxTests/FileExplorerStoreTests.swift:66) - In testExpandedRemoteNodesHydrateWhenProviderBecomesAvailable, wait for the provider to actually be consulted, not just for isRootLoading == false which can be true before the unstructured load Task has started (CodeRabbit, line 154) - Drop redundant srcNode.children = nil in testStaleErrorClearsOnRetry; the error path in loadChildren never assigns children (Greptile, line 244) --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This branch was successfully deployed
1 participant
Summary
The
macOS Compatibilityworkflow has been flaky onwarp-macos-15-arm64-6xsince PR #1963 was merged (commit 6d7bbd2, author: me). The failure surfaced again on main after PR #2875 and the user flagged it started failing around commit ab46c55 too.Root cause:
cmuxTests/FileExplorerStoreTests.swiftused fixedTask.sleep(nanoseconds: 100_000_000)waits to let the unstructuredTask { ... }started bysetRootPath/expand/setProviderhop to@MainActorand mutate@Publishedstate. On the slower macOS 15 warp runner, 100ms isn't enough —rootNodesis still empty, andtestExpandedNodesSurviveStoreRecreationline 180 force-unwrapsstore.rootNodes.first { \$0.name == \"lib\" }!and crashes the test runner. macOS 26 always wins the race, so the older runner alone hangs until the 30m job timeout.Fix: replace every fixed sleep with a polling
waitFor(...)helper that returns as soon as the observable state (rootNodes,children,error,isRootLoading) matches the expected condition, with a 5s safety timeout. Also pin the test class to@MainActorso reads and writes of@Publishedstate share an actor instead of racing across threads.Test plan
cmux-unitscheme builds locallyFileExplorerStoreTestspass locally in 0.34s total (vs. ~1.2s of sleeps before)macOS Compatibilitypasses on bothwarp-macos-26-arm64-6xandwarp-macos-15-arm64-6xin CISummary by cubic
Stabilizes
FileExplorerStoreTestsby replacing fixed sleeps with a pollingwaitFor(...)that throws on timeout and marking the test class@MainActor. This removes race conditions on slower macOS runners and prevents force-unwrap crashes.waitFor(...)now throws on timeout to stop tests before unsafe fallthroughs.isRootLoading == false.srcNode.children = nilin the stale error retry path.Written for commit 03dc505. Summary will update on new commits.
Summary by CodeRabbit