Repository navigation
Wake E2E display before strict activation test - #4942
lawrencecchen wants to merge 2 commits into
Conversation
|
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.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves ChangesUI Test App Launch and Activation Failure Handling
CI Runner and Guard Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR removes
Confidence Score: 5/5Safe to merge — the change is confined to test scaffolding and intentionally tightens CI signal by removing activation leniency. The diff touches only one UI test helper. The logic is straightforward: launch strictly, poll for foreground, fail hard if not reached. The old silent path through .runningBackground is correctly removed, and the guard plus continueAfterFailure=false ensures no further UI interactions run on a mis-activated app. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[launchAndActivate called] --> B[app.launch]
B --> C{runningForeground?}
C -- yes --> D[return early]
C -- no --> E[pollUntil 4s loop calling app.activate]
E --> F{reachedForeground?}
F -- yes --> G[return normally]
F -- no --> H[XCTFail with state value]
H --> I[return - test stops via continueAfterFailure=false]
Reviews (6): Last reviewed commit: "Remove CommandPalette activation expecte..." | Re-trigger Greptile |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
6b7da97 to
81f0233
Compare
lawrencecchen
changed the title
81f0233 to
748755a
Compare
lawrencecchen
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 748755a. Configure here.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-e2e.yml:
- Around line 286-291: The new trap line is overriding the earlier EXIT trap and
preventing MANIFEST_PATH cleanup; remove the trap 'kill "$CAFFEINATE_PID" …'
invocation (or instead append to the existing EXIT trap rather than replacing
it) so the original EXIT handler from earlier in the script is preserved; locate
the caffeinate usage and CAFFEINATE_PID variable and either delete the trap
command or implement appending to the existing EXIT trap for cleanup rather than
setting a second trap.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bcd93be2-45f5-400b-8053-ce8014e39050
📒 Files selected for processing (3)
.github/workflows/test-e2e.ymlcmuxUITests/CommandPaletteIdentifierClipboardUITests.swiftcmuxUITests/UpdatePillUITests.swift
| if command -v caffeinate >/dev/null 2>&1; then | ||
| echo "Keeping display awake during xcodebuild" | ||
| caffeinate -dim -w $$ >/tmp/cmux-e2e-caffeinate.log 2>&1 & | ||
| CAFFEINATE_PID=$! | ||
| trap 'kill "$CAFFEINATE_PID" 2>/dev/null || true' EXIT | ||
| caffeinate -u -t 5 >/dev/null 2>&1 || true |
There was a problem hiding this comment.
Preserve existing EXIT trap when enabling caffeinate.
At Line 290, this trap overrides the earlier EXIT trap from Line 249, so MANIFEST_PATH cleanup can be skipped in the display-regression branch. Keep existing trap behavior or avoid setting a second EXIT trap here.
Suggested fix
if command -v caffeinate >/dev/null 2>&1; then
echo "Keeping display awake during xcodebuild"
caffeinate -dim -w $$ >/tmp/cmux-e2e-caffeinate.log 2>&1 &
CAFFEINATE_PID=$!
- trap 'kill "$CAFFEINATE_PID" 2>/dev/null || true' EXIT
caffeinate -u -t 5 >/dev/null 2>&1 || true
ficaffeinate -w $$ already exits when the current shell exits, so this avoids clobbering prior EXIT cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test-e2e.yml around lines 286 - 291, The new trap line is
overriding the earlier EXIT trap and preventing MANIFEST_PATH cleanup; remove
the trap 'kill "$CAFFEINATE_PID" …' invocation (or instead append to the
existing EXIT trap rather than replacing it) so the original EXIT handler from
earlier in the script is preserved; locate the caffeinate usage and
CAFFEINATE_PID variable and either delete the trap command or implement
appending to the existing EXIT trap for cleanup rather than setting a second
trap.
748755a to
517e325
Compare
lawrencecchen
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| echo "Keeping display awake during xcodebuild" | ||
| caffeinate -dim -w $$ >/tmp/cmux-e2e-caffeinate.log 2>&1 & | ||
| CAFFEINATE_PID=$! | ||
| trap 'kill "$CAFFEINATE_PID" 2>/dev/null || true' EXIT |
There was a problem hiding this comment.
When TEST_FILTER=DisplayResolutionRegressionUITests, line 249 installs trap 'rm -f "$MANIFEST_PATH"' EXIT. On macOS, caffeinate is always present, so the new trap 'kill "$CAFFEINATE_PID" 2>/dev/null || true' EXIT unconditionally overwrites that earlier trap. The manifest JSON in /tmp is then never removed on exit for that test filter.
Note that the caffeinate trap is actually redundant: caffeinate -dim -w $$ already causes caffeinate to exit as soon as the watched shell PID dies, so no explicit kill is needed. Removing the trap line entirely avoids the clobber while preserving the caffeinate wake behaviour.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/test-e2e.yml (1)
290-290:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDo not overwrite the existing EXIT trap.
At Line 290, this
trapreplaces the earlier EXIT handler from Line 249, soMANIFEST_PATHcleanup can be skipped in display-regression runs. Prefer relying oncaffeinate -w $$lifecycle (or append safely), not replacing the trap.Suggested fix
if command -v caffeinate >/dev/null 2>&1; then echo "Keeping display awake during xcodebuild" caffeinate -dim -w $$ >/tmp/cmux-e2e-caffeinate.log 2>&1 & - CAFFEINATE_PID=$! - trap 'kill "$CAFFEINATE_PID" 2>/dev/null || true' EXIT caffeinate -u -t 5 >/dev/null 2>&1 || true fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test-e2e.yml at line 290, The new trap for CAFFEINATE_PID overwrites the earlier EXIT handler (which performs MANIFEST_PATH cleanup), so change it to append instead of replace: capture the existing trap into a variable (e.g. PREV_TRAP="$(trap -p EXIT | sed -E "s/trap -- '(.+)' EXIT/\1/")" or similar), then install a combined EXIT handler that first kills "$CAFFEINATE_PID" (kill "$CAFFEINATE_PID" 2>/dev/null || true) and then invokes the previous handler; alternatively rely on caffeinate -w $$ lifecycle and remove the replacing trap so you do not discard the original cleanup logic for MANIFEST_PATH.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/test-e2e.yml:
- Line 290: The new trap for CAFFEINATE_PID overwrites the earlier EXIT handler
(which performs MANIFEST_PATH cleanup), so change it to append instead of
replace: capture the existing trap into a variable (e.g. PREV_TRAP="$(trap -p
EXIT | sed -E "s/trap -- '(.+)' EXIT/\1/")" or similar), then install a combined
EXIT handler that first kills "$CAFFEINATE_PID" (kill "$CAFFEINATE_PID"
2>/dev/null || true) and then invokes the previous handler; alternatively rely
on caffeinate -w $$ lifecycle and remove the replacing trap so you do not
discard the original cleanup logic for MANIFEST_PATH.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08e0edd9-e2bf-43d3-b545-d5b9dc0e238d
📒 Files selected for processing (2)
.github/workflows/test-e2e.ymlcmuxUITests/CommandPaletteIdentifierClipboardUITests.swift
517e325 to
ae575a5
Compare
|
Actionable comments posted: 0 |
ae575a5 to
3f3af50
Compare
|
Closing this PR because the strict CommandPalette activation change failed its exact hosted proof run. After the runner-label cleanup, the focused run still landed on a true Depot runner (depot-w8l2gw3nfv, console GUI user runner, Xcode 16.4) and both CommandPaletteIdentifierClipboardUITests cases failed with |
This branch was successfully deployed
1 participant
Adds a display wake/prevent-sleep guard before hosted E2E xcodebuild, then removes the CommandPalette activation expected-failure wrappers as the strict activation proof case.
Why this scope: after #4940, the
depot-macos-latestlabel can still land oncmux-aws-m4pro-*runners whose display is asleep. Strict launch runs on those machines still failed withRunning Background, so this PR wakes and keeps the display awake during xcodebuild. Other masks stay in place until their behavior-level class failures or runner-specific activation failures are handled separately.Validation:
git diff --checkactionlint -oneline .github/workflows/test-e2e.yml./tests/test_ci_self_hosted_guard.shSummary by CodeRabbit
Tests
Chores
Note
Low Risk
Changes only affect CI workflow and UI test expectations, not production app logic or user data.
Overview
Hosted E2E on
depot-macos-latestcan run on self-hosted runners whose display is asleep, so strict UI tests still saw the app stuck inRunning Backgroundafter launch. This PR adds a display wake / keep-awake step aroundxcodebuildin.github/workflows/test-e2e.ymlso the GUI session stays active for the test run.Command palette UI tests drop
XCTExpectFailurewrappers around app activation, so launch must reach the foreground on those runners instead of being tolerated as a known CI flake.Reviewed by Cursor Bugbot for commit 068d872. Bugbot is set up for automated code reviews on this repo. Configure here.