Repository navigation
Re-enable FileExplorerStore XCTest in CI - #5682
Conversation
|
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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR removes a specific XCTest method from the CI workflow's skip list, causing it to execute during unit tests. It simultaneously adds a regression guard script that prevents any ChangesXCTest Skip Removal and Guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (18 passed)
✨ 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 |
Greptile SummaryRe-enables the previously quarantined
Confidence Score: 4/5Safe to merge; both changes are CI-only with no production code impact, and the existing 900 s timeout provides a backstop if the re-enabled test hangs again. The one-line removal in ci.yml is straightforward, and the guard script addition is a net improvement. The only real concern is the guard's grep pattern matching YAML comment lines that mention -skip-testing:, which could cause spurious failures if documentation comments are ever added to ci.yml. tests/test_ci_self_hosted_guard.sh — the grep pattern in check_no_ci_xctest_skips warrants a second look for the comment false-positive edge case. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CI: Run unit tests step] --> B[xcodebuild_noninteractive.py]
B --> C[xcodebuild -scheme cmux-unit test]
C --> D{FileExplorerStoreTests\ntestRemoteWorkspaceRoot...}
D -->|Now included| E[Test executes on macOS runner]
E --> F{Pass / Fail?}
F -->|Pass| G[xcodebuild exits 0]
F -->|Hang| H[Timeout loop 900 s]
H --> I[kill -TERM / kill -KILL]
I --> J[return 124]
G --> K[CI job succeeds]
subgraph Guard
L[check_no_ci_xctest_skips] --> M{grep -nE -skip-testing: in ci.yml}
M -->|Found| N[FAIL + exit 1]
M -->|Not found| O[PASS]
end
Reviews (1): Last reviewed commit: "Re-enable FileExplorerStore XCTest in CI" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| check_no_ci_xctest_skips() { | ||
| if grep -nE '(^|[[:space:]])-skip-testing:' "$CI_FILE"; then |
There was a problem hiding this comment.
-skip-testing:
The pattern (^|[[:space:]])-skip-testing: will also fire on any YAML comment that mentions the flag, e.g. a line like # previously excluded with -skip-testing:.... If a developer ever adds such a comment for documentation purposes — a likely maintenance scenario given the PR description itself calls out the historical quarantine — the guard fails even though no actual xcodebuild exclusion exists. Consider piping through a second grep -v to drop comment-only lines before evaluating the match.
| if grep -nE '(^|[[:space:]])-skip-testing:' "$CI_FILE"; then | |
| if grep -nE '(^|[[:space:]])-skip-testing:' "$CI_FILE" | grep -qv '^[[:space:]]*#'; then |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This branch was successfully deployed
1 participant
Restores coverage for the last explicit XCTest method exclusion in
.github/workflows/ci.yml.Flaky path:
CI/testscmuxTests/FileExplorerStoreTests/testRemoteWorkspaceRootRequestResolvesSSHHomeInsteadOfKeepingLocalPath-skip-testing:. The original quarantine came from app-host unit runner hangs and post-test XCTest cleanup issues.Coverage preserved:
-skip-testing:exclusion from theRun unit testsxcodebuild invocation.ci.ymlgrows another-skip-testing:XCTest quarantine.Before:
mainCI runs execute thetestsjob, but this one method is excluded.maintestsjobs with the exclusion: 42m13s, 24m04s, 25m05s, 21m18s, 22m10s from GitHub Actions run list..github/workflows/ci.yml, GitHub Actions CI run history.After:
Local verification:
bash tests/test_ci_self_hosted_guard.shgit diff --checkRelated context:
No tagged reload: CI/workflow-only change.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Workflow-only change; main risk is CI flakiness or hangs if the previously quarantined app-host test still misbehaves on macOS runners.
Overview
Removes the
-skip-testing:quarantine forFileExplorerStoreTests/testRemoteWorkspaceRootRequestResolvesSSHHomeInsteadOfKeepingLocalPathso that method runs again in thetestsjob’s fullcmux-unitxcodebuild testinvocation.Adds
check_no_ci_xctest_skipsintest_ci_self_hosted_guard.sh(already run byworkflow-guard-tests) soci.ymlcannot reintroduce per-method XCTest exclusions via-skip-testing:.Reviewed by Cursor Bugbot for commit 0ea3e1f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Re-enabled the excluded XCTest in CI by removing the
-skip-testing:forcmuxTests/FileExplorerStoreTests/testRemoteWorkspaceRootRequestResolvesSSHHomeInsteadOfKeepingLocalPath. Added a guard intests/test_ci_self_hosted_guard.shthat fails if.github/workflows/ci.ymlcontains any-skip-testing:entries.Written for commit 0ea3e1f. Summary will update on new commits.
Summary by CodeRabbit