Skip to content

Unmask right sidebar chrome E2E tests - #4949

Merged
lawrencecchen merged 2 commits into
mainfrom
feat-e2e-expect-failure-audit
May 28, 2026
Merged

lawrencecchen merged 2 commits into
mainfrom
feat-e2e-expect-failure-audit

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented May 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • remove non-strict expected-failure launch wrappers from RightSidebarChromeHeightUITests
  • update stale chrome assertions so visual 28 pt metrics are not compared to 48 pt accessibility hit targets
  • cover the same stale assertion in BonsplitTabDragUITests

Verification


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.


Note

Low Risk
Test-only changes to XCTest expectations and launch handling; no production UI or app logic.

Overview
Unmasks right-sidebar chrome E2E tests so launch failures surface on CI instead of being swallowed by non-strict XCTExpectFailure around app.launch() in RightSidebarChromeHeightUITests.

Fixes outdated height checks that compared 28 pt in-app chrome metrics to Bonsplit pane tab frames from XCUITest. Those elements can be taller because of accessibility hit targets (~48 pt). Assertions now require the tab’s frame height to be at least the compact lane height, not equal to it—the same change in BonsplitTabDragUITests for the cross-presentation-mode mode-bar test.

Reviewed by Cursor Bugbot for commit 527aca4. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Unmasked the right sidebar chrome UI tests and fixed hit-target assertions so we compare 28 pt chrome lanes against 48 pt tab targets. Improves reliability on CI and aligns tests with intended visual vs accessibility metrics.

  • Bug Fixes
    • Removed non-strict expected-failure wrappers; launch app directly in RightSidebarChromeHeightUITests.
    • Replaced equality checks with greater-or-equal for tab hit target vs 28 pt chrome lane.
    • Added matching assertion coverage to BonsplitTabDragUITests.

Written for commit 527aca4. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

Release Notes

  • Tests
    • Improved test stability by removing app launch failure expectations on headless CI runners.
    • Enhanced UI height verification tests with more flexible assertions to ensure proper hit-target coverage for interface elements across different presentation modes.

Review Change Stack

@vercel

vercel Bot commented May 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment May 28, 2026 1:18pm
cmux-staging Ready Ready Preview, Comment May 28, 2026 1:18pm

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented May 28, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ebb6e902-aa20-4662-ba83-05d465418299

📥 Commits

Reviewing files that changed from the base of the PR and between b1a68e9 and 527aca4.

📒 Files selected for processing (2)
  • cmuxUITests/BonsplitTabDragUITests.swift
  • cmuxUITests/RightSidebarChromeHeightUITests.swift

📝 Walkthrough

Walkthrough

This PR adjusts two UI test files to relax assertion strictness and improve test robustness. App launch is no longer treated as an expected failure, and sidebar chrome height assertions are loosened from exact equality to minimum coverage checks.

Changes

UI Test Assertion Relaxation

Layer / File(s) Summary
App launch failure expectations removed
cmuxUITests/RightSidebarChromeHeightUITests.swift
XCTExpectFailure wrappers around app.launch() are removed from both test cases, making app activation a strict requirement instead of an anticipated failure on headless CI runners.
Chrome and tab height assertions relaxed to hit-target checks
cmuxUITests/BonsplitTabDragUITests.swift, cmuxUITests/RightSidebarChromeHeightUITests.swift
Height assertions are changed from strict equality to >= checks: pane tab frame height and secondary bar height must satisfy minimum hit-target coverage rather than exact match requirements.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

Possibly related issues

Possibly related PRs

  • manaflow-ai/cmux#4940: Both PRs remove non-strict XCTExpectFailure wrappers around app.launch() across different UI test files to enforce strict app activation success.
  • manaflow-ai/cmux#4928: Both PRs modify RightSidebarChromeHeightUITests by removing the XCTExpectFailure wrapper around app launch and adjusting related test assertions.

Poem

🐰 The tests grow stronger with each tweak and care,
No longer expecting failures in the air—
Heights now measured by their coverage true,
Hit targets met, with >= checking through!

🚥 Pre-merge checks | ✅ 17 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (17 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and concisely describes the main change: unmasking (removing expected-failure wrappers from) right sidebar chrome E2E tests.
Description check ✅ Passed The description includes a clear Summary section explaining changes and includes verification links to baseline and passing test runs, but omits the Testing, Demo Video, Review Trigger, and Checklist sections from the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed PR modifies only test files (UITests directory). Per custom check rules, test changes are explicitly allowed and PASS the Swift actor isolation check.
Cmux Swift Blocking Runtime ✅ Passed Changes are test-only (cmuxUITests) with no blocking runtime primitives; only removes XCTExpectFailure wrappers and changes assertions from equality to GreaterThanOrEqual.
Cmux No Hacky Sleeps ✅ Passed This PR only modifies Swift test files. The rule explicitly scopes to TypeScript, JavaScript, shell, and non-Swift build/runtime scripts. Swift primitives are covered by "swift-blocking-runtime.md".
Cmux Algorithmic Complexity ✅ Passed All modified files are UI tests (XCTestCase subclasses), not production code. Per rules, test scaffolding passes. No algorithmic complexity violations found.
Cmux Swift Concurrency ✅ Passed Changes to XCTest UI test files do not introduce legacy async patterns; only uses allowed XCTest boundaries and test synchronization primitives.
Cmux Swift @Concurrent ✅ Passed PR modifies only UI test assertion logic in synchronous XCTest methods with no async functions, @concurrent annotations, or actor isolation present.
Cmux Swift File And Package Boundaries ✅ Passed Test file changes are allowed exceptions: incidental touches to oversized BonsplitTabDragUITests (+3/-4) and focused bug fixes to RightSidebarChromeHeightUITests (+3/-11) fixing stale assertions.
Cmux Swift Logging ✅ Passed PR only modifies UI test files with no logging additions; test files are explicitly exempt from logging rules per swift-logging.md.
Cmux User-Facing Error Privacy ✅ Passed Changes are in test files only (cmuxUITests directory). Per the user-facing-errors.md rule, tests are explicitly allowed cases that do not require checking for privacy violations.
Cmux Full Internationalization ✅ Passed PR modifies only UITest files with developer-facing test assertions, not user-facing production code; test changes are explicitly excepted from internationalization rules.
Cmux Swiftui State Layout ✅ Passed PR contains only UI test code changes with no SwiftUI state, layout, or observation pattern modifications; check is not applicable to test-only changes.
Cmux Architecture Rethink ✅ Passed All changes are test-only UI test fixes. No production code changes, timing repairs, locks, observers, or architectural violations. Per guidelines, test-only changes are allowed.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed Changes only affect test-only UITest fixtures verifying existing app behavior via accessibility queries, not window creation code.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-e2e-expect-failure-audit

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR unblocks RightSidebarChromeHeightUITests and BonsplitTabDragUITests by removing the non-strict XCTExpectedFailure launch wrappers and fixing two stale equality assertions that were comparing the 28 pt visual chrome lane height against the larger 48 pt accessibility hit-target frame.

  • Removed XCTExpectedFailure(isStrict: false) from both test methods in RightSidebarChromeHeightUITests, replacing the guarded app.launch() with a direct call so launch failures are now surfaced as real test failures.
  • Changed XCTAssertEqual(..., accuracy: 2) → XCTAssertGreaterThanOrEqual(alphaTab.frame.height, chromeHeight) in both test files, correctly encoding the invariant that the hit target must cover the compact chrome lane rather than match it pixel-for-pixel.

Confidence Score: 5/5

Test-only changes that remove masking wrappers and correct stale assertions; no production code is touched.

Both files are UI test scaffolding. The removed XCTExpectedFailure wrappers were silently swallowing launch failures; removing them makes the suite correctly surface regressions. The assertion direction fix (>= instead of ==) accurately captures the intended hit-target invariant. No logic outside the test target is affected.

No files require special attention.

Important Files Changed

Filename Overview
cmuxUITests/RightSidebarChromeHeightUITests.swift Removes two non-strict XCTExpectedFailure wrappers around app.launch() and updates one XCTAssertEqual to XCTAssertGreaterThanOrEqual, correctly reflecting that the accessibility hit target must cover (>=) the 28 pt visual chrome lane rather than matching it exactly.
cmuxUITests/BonsplitTabDragUITests.swift Mirrors the assertion correction from RightSidebarChromeHeightUITests: replaces XCTAssertEqual(modeBarHeight, alphaTab.frame.height, accuracy:2) with XCTAssertGreaterThanOrEqual(alphaTab.frame.height, modeBarHeight), aligning the check with the 48 pt hit-target vs 28 pt visual lane distinction.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[app.launch] --> B[app.state check]
    B -- runningBackground --> C[app.activate]
    B -- other --> D[wait for runningForeground]
    C --> D
    D --> E[waitForJSONKey ready equals 1]
    E --> F[XCTAssertEqual secondaryBarHeight == 28pt]
    F --> G[XCTAssertGreaterThanOrEqual alphaTab.frame.height >= secondaryBarHeight]
    G --> H[assert control heights match modeControlHeight]
    H --> I[test passes]
    style G fill:#d4edda,stroke:#28a745
Loading

Reviews (1): Last reviewed commit: "Fix right sidebar chrome hit-target asse..." | Re-trigger Greptile

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown

Actionable comments posted: 0

@lawrencecchen
lawrencecchen merged commit a14cc29 into main May 28, 2026
18 checks passed
@lawrencecchen
lawrencecchen deleted the feat-e2e-expect-failure-audit branch May 28, 2026 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants