E2E: Downloads coverage - #3859
Conversation
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds e2e helper modules (downloads.ts, downloadsDropdown.ts) for Playwright/Electron download testing, including a local HTTP server, app launching, dropdown interaction, and state polling utilities. New test specs cover download cancellation, clear-all, opening, and video downloads. Existing specs are refactored to use a shared closeElectronAppFast teardown helper. ChangesDownload E2E Helpers and Tests
Sequence Diagram(s)sequenceDiagram
participant Test
participant DownloadServer
participant ElectronApp
participant Popup
participant DownloadsJson as downloads.json
Test->>DownloadServer: startDownloadServer/startVideoServer
Test->>ElectronApp: launchAppWithDownloadsDir
Test->>Popup: triggerDownloadFromPopup (click download-link)
Popup->>DownloadsJson: write download state (progressing/completed)
Test->>DownloadsJson: waitForDownloadState / poll
Test->>ElectronApp: openDownloadsDropdown / cancel / clear all
ElectronApp->>DownloadsJson: update state on cancel/clear
Test->>ElectronApp: closeDownloadTestApp / closeElectronAppFast
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yasserfaraazkhan
changed the title
Resolve harness/hook conflicts by keeping merged #3855 changes. Co-authored-by: Cursor <cursoragent@cursor.com>
|
❌ E2E Test Setup Failed Failed to create E2E test instances: installation wait cancelled: context canceled |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
e2e/specs/downloads/download_cancel.test.ts (1)
38-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting menu-window discovery into a shared helper.
This window-discovery pattern (find existing window, else
waitForEventwith predicate/timeout) is the kind of logic the helpers module exists for, and is likely needed by other download specs (e.g. the open/cancel flows) that also interact with the dropdown menu window. Extracting it intoe2e/helpers/downloads.ts(e.g.getDownloadsDropdownMenuWindow(app)) avoids duplicating this logic across multiple spec files.As per coding guidelines, "Reuse helpers from
e2e/helpersbefore creating new launch, login, or server-discovery logic."🤖 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 `@e2e/specs/downloads/download_cancel.test.ts` around lines 38 - 44, Extract the downloads dropdown menu window lookup into a shared helper in the downloads helpers module, since the current find-or-wait pattern is duplicated and likely needed across multiple download specs. Add a reusable helper such as getDownloadsDropdownMenuWindow(app) in e2e/helpers/downloads.ts that encapsulates the existing app.windows() search plus app.waitForEvent('window', { predicate, timeout }) fallback, and update the download cancel flow to call that helper instead of inlining the discovery logic. Also reuse the same helper from the related open/cancel spec paths where the downloads dropdown menu window is needed.Source: Coding guidelines
e2e/specs/downloads/video_download.test.ts (3)
127-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePopup reference stored on an ad-hoc global instead of the sanctioned
__e2eTestRefs.
(global as any).__videoDownloadPopup = popupintroduces a one-off global that's never read elsewhere, instead of using the establishedglobal.__e2eTestRefschannel for main-process state exchange.As per path instructions, "Use
app.evaluate()only to read/write main-process state viaglobal.__e2eTestRefsorglobal.__e2eAppReady."🤖 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 `@e2e/specs/downloads/video_download.test.ts` around lines 127 - 136, The popup reference is being written to an ad-hoc global in the `app.evaluate()` block instead of the approved main-process test refs channel. Update the `videoDownload` popup setup to store the `BrowserWindow` on `global.__e2eTestRefs` (matching the existing e2e state-sharing pattern) rather than using `(global as any).__videoDownloadPopup`, and keep the `app.evaluate()` usage limited to the sanctioned globals. Make sure the unique popup creation flow in the `video_download.test.ts` scenario still assigns and later accesses the reference through `__e2eTestRefs`.Source: Path instructions
34-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the shared download-server helper for the MP4 case
e2e/helpers/downloads.tshardcodes/download.txtandtext/plain, so this test can’t reuse it as-is. If this flow should share the helper, make it accept a custom path, content type, and body; otherwise the local server here is the right approach.🤖 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 `@e2e/specs/downloads/video_download.test.ts` around lines 34 - 69, The MP4 download test is currently using a bespoke local server because the shared download helper is hardcoded for a text file path and MIME type. Update the shared helper in downloads.ts so its server setup can accept a custom request path, content type, and body, then reuse that from startVideoServer if this flow should share the common setup; otherwise keep the local server and avoid trying to force the existing helper to handle only /download.txt and text/plain.Source: Path instructions
101-118: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReuse the download helper and centralize launch flags. This spec already has per-test
downloadLocationsetup, so keep that inlaunchAppWithDownloadsDirandcloseDownloadTestAppinstead of hand-rollingelectron.launchhere; then add the CI-hardening flags once ine2e/helpers/downloads.ts.🤖 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 `@e2e/specs/downloads/video_download.test.ts` around lines 101 - 118, The video download spec is bypassing the shared download app helper and duplicating Electron launch setup. Move this setup to use the existing download helper functions, keeping per-test downloadLocation handling inside launchAppWithDownloadsDir and closeDownloadTestApp, and centralize the CI-hardening launch flags in e2e/helpers/downloads.ts so the spec no longer calls electron.launch directly.Source: Path instructions
🤖 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 `@e2e/helpers/downloads.ts`:
- Around line 217-250: The teardown in closeDownloadTestApp still uses the
slower app.close() plus waitForLockFileRelease flow, which should be replaced by
the shared closeElectronAppFast helper used across the updated e2e specs. Update
closeDownloadTestApp to delegate the ElectronApplication shutdown to
closeElectronAppFast (keeping the popup cleanup and download directory removal
logic intact) so this helper matches the new standardized fast teardown path.
- Around line 94-101: The local download server startup in the download helper
can crash on an unhandled http.Server error instead of rejecting cleanly. In the
setup around server.listen() and the address check, add an error listener on the
server before calling listen, and ensure any startup failure rejects the promise
so callers can handle it. Use the existing download server initialization flow
in downloads.ts to keep the fix scoped to the server startup path.
In `@e2e/helpers/downloadsDropdown.ts`:
- Around line 10-39: The helper closeDownloadsDropdownIfOpen is using
BrowserWindow.getAllWindows() to find downloadsDropdown.html, but that view is
hosted in MainWindow.contentView via a WebContentsView, so it will never be
discovered and the dropdown may remain open. Replace the window-scanning logic
with the existing close IPC/path used by the downloads dropdown, using the
relevant main-process close handler or message that already closes the downloads
UI. Keep the retry/timeout behavior if needed, but make the helper trigger the
real close flow instead of inspecting BrowserWindow webContents URLs.
In `@e2e/specs/downloads/download_cancel.test.ts`:
- Around line 52-55: The cleanup in the test teardown currently lets a failure
in closeDownloadTestApp prevent the local download server from being closed, so
update the finally block in downloads/download_cancel.test.ts to make close()
run even if closeDownloadTestApp(app, userDataDir, downloadLocation) throws. Use
separate teardown handling around closeDownloadTestApp and close so the server
shutdown is guaranteed, and keep the logic localized to the existing finally
cleanup path.
In `@e2e/specs/downloads/download_open.test.ts`:
- Around line 32-38: The test hook in the download-open spec is using a new
ad-hoc global for captured paths instead of the repo’s standard main-process
test state container. Update the `app.evaluate` setup to store and read the
`shell.openPath` call history through `global.__e2eTestRefs` (consistent with
`__e2eAppReady` patterns) and keep the hook wiring in the same place so other
specs can discover and reuse it.
---
Nitpick comments:
In `@e2e/specs/downloads/download_cancel.test.ts`:
- Around line 38-44: Extract the downloads dropdown menu window lookup into a
shared helper in the downloads helpers module, since the current find-or-wait
pattern is duplicated and likely needed across multiple download specs. Add a
reusable helper such as getDownloadsDropdownMenuWindow(app) in
e2e/helpers/downloads.ts that encapsulates the existing app.windows() search
plus app.waitForEvent('window', { predicate, timeout }) fallback, and update the
download cancel flow to call that helper instead of inlining the discovery
logic. Also reuse the same helper from the related open/cancel spec paths where
the downloads dropdown menu window is needed.
In `@e2e/specs/downloads/video_download.test.ts`:
- Around line 127-136: The popup reference is being written to an ad-hoc global
in the `app.evaluate()` block instead of the approved main-process test refs
channel. Update the `videoDownload` popup setup to store the `BrowserWindow` on
`global.__e2eTestRefs` (matching the existing e2e state-sharing pattern) rather
than using `(global as any).__videoDownloadPopup`, and keep the `app.evaluate()`
usage limited to the sanctioned globals. Make sure the unique popup creation
flow in the `video_download.test.ts` scenario still assigns and later accesses
the reference through `__e2eTestRefs`.
- Around line 34-69: The MP4 download test is currently using a bespoke local
server because the shared download helper is hardcoded for a text file path and
MIME type. Update the shared helper in downloads.ts so its server setup can
accept a custom request path, content type, and body, then reuse that from
startVideoServer if this flow should share the common setup; otherwise keep the
local server and avoid trying to force the existing helper to handle only
/download.txt and text/plain.
- Around line 101-118: The video download spec is bypassing the shared download
app helper and duplicating Electron launch setup. Move this setup to use the
existing download helper functions, keeping per-test downloadLocation handling
inside launchAppWithDownloadsDir and closeDownloadTestApp, and centralize the
CI-hardening launch flags in e2e/helpers/downloads.ts so the spec no longer
calls electron.launch directly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce31ab03-49af-496a-b78e-17184a8a0e96
📒 Files selected for processing (10)
e2e/helpers/downloads.tse2e/helpers/downloadsDropdown.tse2e/specs/downloads/download_cancel.test.tse2e/specs/downloads/download_clear_all.test.tse2e/specs/downloads/download_completion.test.tse2e/specs/downloads/download_open.test.tse2e/specs/downloads/downloads_dropdown_items.test.tse2e/specs/downloads/downloads_manager.test.tse2e/specs/downloads/downloads_menubar.test.tse2e/specs/downloads/video_download.test.ts
Handle download server bind errors, use closeElectronAppFast, close the dropdown via IPC, guard teardown with allSettled, and store openPath captures on __e2eTestRefs. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
|
❌ E2E Test Setup Failed Failed to create E2E test instances: installation wait cancelled: context canceled |
|
@coderabbitai review |
✅ Action performedReview finished.
|
yasserfaraazkhan
added
E2E/Reset-Servers
0922144
into
e2e/03-mattermost-shell
2 participants
Summary
Stacked PR 5/10 — downloads helpers + all download specs.
Test plan
e2e/specs/downloads/Change Impact: 🟡 Medium
Regression Risk: The changes are mostly isolated to E2E test helpers and download-related specs, but they touch shared test utilities, Electron teardown flows, and persisted download-state handling. Risk is moderate because failures could surface in cleanup, file-lock handling, or download UI behavior across multiple download scenarios.
QA Recommendation: Automated coverage is strong for the added download flows, but a small amount of manual verification is still recommended for the main download, cancel, open, and clear-all paths in the downloads UI.
Generated by CodeRabbitAI