Address CodeRabbit review on downloads E2E helpers and specs.

yasserfaraazkhan · cursoragent · yasserfaraazkhan · commit 6d819080117c · 2026-07-01T06:32:05.000+05:30
Reuse shared download launch/state helpers in video_download, harden server
teardown and popup cleanup, import IPC channel constants, and poll for
dropdown item count in clear-all spec.

Co-authored-by: Cursor &lt;cursoragent@cursor.com&gt;
diff --git a/e2e/helpers/downloads.ts b/e2e/helpers/downloads.ts
@@ -3,6 +3,7 @@
 
 import * as fs from 'fs';
 import * as http from 'http';
+import type {Socket} from 'net';
 import * as path from 'path';
 
 import {expect} from '@playwright/test';
@@ -24,6 +25,7 @@ export async function startDownloadServer(
 ): Promise<DownloadServer> {
     const contents = options?.contents ?? 'download test contents';
     const slow = options?.slow ?? false;
+    const connections = new Set<Socket>();
 
     const server = http.createServer((request, response) => {
         if (request.url === '/download.txt') {
@@ -93,6 +95,11 @@ export async function startDownloadServer(
         `);
     });
 
+    server.on('connection', (socket) => {
+        connections.add(socket);
+        socket.on('close', () => connections.delete(socket));
+    });
+
     await new Promise<void>((resolve, reject) => {
         server.once('error', reject);
         server.listen(0, '127.0.0.1', () => resolve());
@@ -106,6 +113,9 @@ export async function startDownloadServer(
         server,
         url: `http://127.0.0.1:${address.port}`,
         close: () => new Promise<void>((resolve, reject) => {
+            for (const socket of connections) {
+                socket.destroy();
+            }
             server.close((error) => (error ? reject(error) : resolve()));
         }),
     };
@@ -164,6 +174,11 @@ export async function triggerDownloadFromPopup(app: ElectronApplication, popupUr
     });
 
     await app.evaluate(async ({BrowserWindow}, url) => {
+        const existing = (global as any).__e2eDownloadPopup;
+        if (existing && !existing.isDestroyed?.()) {
+            existing.close();
+        }
+
         const popup = new BrowserWindow({
             show: true,
             width: 900,
@@ -228,25 +243,30 @@ export async function closeDownloadTestApp(app: ElectronApplication, userDataDir
 
     await closeElectronAppFast(app, userDataDir).catch(() => {});
 
-    if (!fs.existsSync(downloadLocation)) {
-        return;
-    }
-
-    const timeout = process.platform === 'win32' ? 10_000 : 2_000;
-    const deadline = Date.now() + timeout;
-    let lastError: unknown;
-    while (Date.now() < deadline) {
-        try {
-            fs.rmSync(downloadLocation, {recursive: true, force: true, maxRetries: 3, retryDelay: 200});
+    const removeDirWithRetry = async (dir: string) => {
+        if (!fs.existsSync(dir)) {
             return;
-        } catch (error) {
-            lastError = error;
-            const code = (error as NodeJS.ErrnoException).code;
-            if (code !== 'EBUSY' && code !== 'EPERM' && code !== 'ENOTEMPTY') {
-                throw error;
+        }
+
+        const timeout = process.platform === 'win32' ? 10_000 : 2_000;
+        const deadline = Date.now() + timeout;
+        let lastError: unknown;
+        while (Date.now() < deadline) {
+            try {
+                fs.rmSync(dir, {recursive: true, force: true, maxRetries: 3, retryDelay: 200});
+                return;
+            } catch (error) {
+                lastError = error;
+                const code = (error as NodeJS.ErrnoException).code;
+                if (code !== 'EBUSY' && code !== 'EPERM' && code !== 'ENOTEMPTY') {
+                    throw error;
+                }
+                await new Promise((resolve) => setTimeout(resolve, 250));
             }
-            await new Promise((resolve) => setTimeout(resolve, 250));
         }
-    }
-    throw lastError ?? new Error(`Failed to remove download directory: ${downloadLocation}`);
+        throw lastError ?? new Error(`Failed to remove directory: ${dir}`);
+    };
+
+    await removeDirWithRetry(downloadLocation);
+    await removeDirWithRetry(userDataDir);
 }
diff --git a/e2e/helpers/downloadsDropdown.ts b/e2e/helpers/downloadsDropdown.ts
@@ -3,8 +3,13 @@
 
 import type {ElectronApplication} from 'playwright';
 
-const CLOSE_DOWNLOADS_DROPDOWN = 'close-downloads-dropdown';
-const CLOSE_DOWNLOADS_DROPDOWN_MENU = 'close-downloads-dropdown-menu';
+import {CLOSE_DOWNLOADS_DROPDOWN, CLOSE_DOWNLOADS_DROPDOWN_MENU} from '../../src/common/communication';
+
+function isTransientNavigationError(message: string): boolean {
+    return message.includes('Execution context was destroyed') ||
+        message.includes('Target closed') ||
+        message.includes('Protocol error');
+}
 
 /**
  * Close the downloads dropdown WebContentsView if it is open.
@@ -21,7 +26,7 @@ export async function closeDownloadsDropdownIfOpen(app: ElectronApplication): Pr
             return;
         } catch (error) {
             const message = error instanceof Error ? error.message : String(error);
-            if (!message.includes('Execution context was destroyed')) {
+            if (!isTransientNavigationError(message)) {
                 throw error;
             }
             await new Promise((resolve) => setTimeout(resolve, 100));
diff --git a/e2e/specs/downloads/download_clear_all.test.ts b/e2e/specs/downloads/download_clear_all.test.ts
@@ -52,7 +52,10 @@ test(
         try {
             const {downloadsWindow} = await openDownloadsDropdown(app);
             await downloadsWindow.waitForSelector('.DownloadsDropdown__File', {timeout: 10_000});
-            expect(await downloadsWindow.locator('.DownloadsDropdown__File').count()).toBe(2);
+            await expect.poll(
+                () => downloadsWindow.locator('.DownloadsDropdown__File').count(),
+                {timeout: 10_000},
+            ).toBe(2);
 
             await downloadsWindow.click('.DownloadsDropdown__clearAllButton');
 
diff --git a/e2e/specs/downloads/video_download.test.ts b/e2e/specs/downloads/video_download.test.ts
@@ -6,9 +6,12 @@ import * as http from 'http';
 import * as path from 'path';
 
 import {test, expect} from '../../fixtures/index';
-import {waitForAppReady} from '../../helpers/appReadiness';
-import {electronBinaryPath, appDir, emptyConfig} from '../../helpers/config';
-import {closeElectronAppFast} from '../../helpers/electronApp';
+import {
+    closeDownloadTestApp,
+    launchAppWithDownloadsDir,
+    readDownloadsState,
+    triggerDownloadFromPopup,
+} from '../../helpers/downloads';
 
 // ── MM-T1538: Download a video ────────────────────────────────────────
 // Verifies a real end-to-end download path for a video MIME type:
@@ -17,19 +20,6 @@ import {closeElectronAppFast} from '../../helpers/electronApp';
 //   3. DownloadsManager (src/main/downloadsManager.ts) handles will-download
 //   4. We assert: the file lands on disk AND downloads.json records it
 //      with state "completed"
-//
-// Pattern mirrors download_completion.test.ts so the two tests differ only
-// in MIME type and content — keeping the download flow exercised for the
-// file type MM-T1538 specifically targets (video) without duplicating the
-// scaffolding.
-
-function readJsonFile<T>(filePath: string): T | undefined {
-    try {
-        return JSON.parse(fs.readFileSync(filePath, 'utf-8')) as T;
-    } catch {
-        return undefined;
-    }
-}
 
 async function startVideoServer(filename: string, body: Buffer) {
     const server = http.createServer((request, response) => {
@@ -54,17 +44,20 @@ async function startVideoServer(filename: string, body: Buffer) {
         `);
     });
 
-    await new Promise<void>((resolve) =>
-        server.listen(0, '127.0.0.1', () => resolve()),
-    );
+    await new Promise<void>((resolve, reject) => {
+        server.once('error', reject);
+        server.listen(0, '127.0.0.1', () => resolve());
+    });
     const address = server.address();
     if (!address || typeof address === 'string') {
         throw new Error('Failed to start local video download server');
     }
 
     return {
-        server,
         url: `http://127.0.0.1:${address.port}`,
+        close: () => new Promise<void>((resolve, reject) => {
+            server.close((error) => (error ? reject(error) : resolve()));
+        }),
     };
 }
 
@@ -74,100 +67,31 @@ test(
     async ({}, testInfo) => {
         const filename = 'sample-video.mp4';
 
-        // Minimal .mp4 — magic bytes are enough for the download-manager
-        // pipeline; we never play the file, only assert it landed on disk.
         const videoBody = Buffer.from([
             0x00, 0x00, 0x00, 0x18, 0x66, 0x74, 0x79, 0x70,
             0x6d, 0x70, 0x34, 0x32, 0x00, 0x00, 0x00, 0x00,
             0x6d, 0x70, 0x34, 0x32, 0x69, 0x73, 0x6f, 0x6d,
         ]);
 
-        const {server, url} = await startVideoServer(filename, videoBody);
+        const {url, close: closeVideoServer} = await startVideoServer(filename, videoBody);
 
         const userDataDir = path.join(testInfo.outputDir, 'userdata');
-        const downloadsDir = path.join(testInfo.outputDir, 'Downloads');
-        const config = {
-            ...emptyConfig,
-            downloadLocation: downloadsDir,
-        };
-
-        fs.mkdirSync(userDataDir, {recursive: true});
-        fs.mkdirSync(downloadsDir, {recursive: true});
-        fs.writeFileSync(
-            path.join(userDataDir, 'config.json'),
-            JSON.stringify(config),
-        );
-
-        const {_electron: electron} = await import('playwright');
-        const app = await electron.launch({
-            executablePath: electronBinaryPath,
-            args: [
-                appDir,
-                `--user-data-dir=${userDataDir}`,
-                '--no-sandbox',
-                '--disable-gpu',
-            ],
-            env: {...process.env, NODE_ENV: 'test'},
-            timeout: 60_000,
-        });
-
-        const savedPath = path.join(downloadsDir, filename);
+        const downloadLocation = path.join(testInfo.outputDir, 'Downloads');
+        const app = await launchAppWithDownloadsDir(userDataDir, downloadLocation);
+        const savedPath = path.join(downloadLocation, filename);
 
         try {
-            await waitForAppReady(app);
-            const mainWindow = app.windows().find((window) => window.url().includes('index'));
-            expect(mainWindow).toBeDefined();
-            await mainWindow!.waitForLoadState();
-
-            const popupPromise = app.waitForEvent('window', {
-                predicate: (window) => window.url().startsWith(url),
-                timeout: 15_000,
-            });
-
-            await app.evaluate(async ({BrowserWindow}, popupUrl) => {
-                const popup = new BrowserWindow({
-                    show: true,
-                    width: 900,
-                    height: 700,
-                });
-                await popup.loadURL(popupUrl);
-                (global as any).__videoDownloadPopup = popup;
-            }, url);
-
-            const popupWindow = await popupPromise;
-            await popupWindow.waitForLoadState();
-            await popupWindow.click('#download-link');
-
-            await expect.
-                poll(() => fs.existsSync(savedPath), {timeout: 15_000}).
-                toBe(true);
-
-            // Verify the downloaded bytes match what we served
-            await expect.
-                poll(() => fs.readFileSync(savedPath).equals(videoBody), {timeout: 15_000}).
-                toBe(true);
-
-            // DownloadsManager must record the download as completed
-            await expect.
-                poll(
-                    () => {
-                        const downloads = readJsonFile<Record<string, {state?: string; mimeType?: string}>>(
-                            path.join(userDataDir, 'downloads.json'),
-                        );
-                        return downloads?.[filename]?.state;
-                    },
-                    {timeout: 15_000},
-                ).
-                toBe('completed');
+            await triggerDownloadFromPopup(app, url);
+
+            await expect.poll(() => fs.existsSync(savedPath), {timeout: 15_000}).toBe(true);
+            await expect.poll(() => fs.readFileSync(savedPath).equals(videoBody), {timeout: 15_000}).toBe(true);
+            await expect.poll(
+                () => readDownloadsState(userDataDir)[filename]?.state,
+                {timeout: 15_000},
+            ).toBe('completed');
         } finally {
-            try {
-                await closeElectronAppFast(app, userDataDir);
-            } finally {
-                await new Promise<void>((resolve, reject) =>
-                    server.close((error) => (error ? reject(error) : resolve())),
-                );
-                fs.rmSync(downloadsDir, {recursive: true, force: true});
-            }
+            await closeDownloadTestApp(app, userDataDir, downloadLocation);
+            await closeVideoServer();
         }
     },
 );
