chore(test): setup browser-based e2e test for `test-dev-server` by hyf0 · Pull Request #7325 · rolldown/rolldown

hyf0 · 2025-12-03T06:59:58Z

This PR is taking too much of time. I will improve in later PRs.

hyf0 · 2025-12-03T07:00:19Z

chore(test): setup browser-based e2e test for test-dev-server #7325 👈 (View in Graphite)
main

How to use the Graphite Merge Queue

Add the label graphite: merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

socket-security · 2025-12-03T07:00:26Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	playwright-chromium@1.57.0
	@testing-library/jest-dom@6.9.1
	playwright@1.57.0

View full report

netlify · 2025-12-03T07:01:28Z

✅ Deploy Preview for rolldown-rs canceled.

Name	Link
🔨 Latest commit	`a9e4e6d`
🔍 Latest deploy log	https://app.netlify.com/projects/rolldown-rs/deploys/6933255402295b0008ed1dff

packages/test-dev-server/tests/package.json

Copilot

Pull request overview

This PR sets up Vitest browser mode for the test-dev-server package, enabling browser-based testing using Playwright. This allows testing of DOM-dependent code in a real browser environment, which is essential for validating client-side functionality.

Key changes:

Upgraded Vitest from 4.0.1 to 4.0.15 in the catalog
Added browser testing dependencies (@vitest/browser, @vitest/browser-playwright, @testing-library packages)
Created browser-specific Vitest configuration and setup files
Added an example browser test demonstrating DOM testing capabilities

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
pnpm-workspace.yaml	Updated vitest catalog version to 4.0.15 and added rxjs to trustPolicyExclude for @vitest/browser-playwright
pnpm-lock.yaml	Updated lockfile with new vitest version and browser testing dependencies
packages/test-dev-server/tests/vitest.browser.config.ts	New Vitest browser configuration using Playwright provider for chromium browser
packages/test-dev-server/tests/vitest-setup.ts	Setup file importing jest-dom matchers for Vitest
packages/test-dev-server/tests/vitest-example/HelloWorld.ts	Example component creating a simple DOM element for testing
packages/test-dev-server/tests/vitest-example/HelloWorld.browser.test.ts	Example browser test using @testing-library/dom
packages/test-dev-server/tests/tsconfig.json	Added vitest/browser types for TypeScript support
packages/test-dev-server/tests/package.json	Added test:browser script and browser testing devDependencies

Files not reviewed (1)

pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

packages/test-dev-server/tests/vitest-example/hello-world.browser.test.ts

packages/test-dev-server/tests/package.json

packages/test-dev-server/tests/tsconfig.json

packages/test-dev-server/tests/package.json

Copilot

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Files not reviewed (1)

pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

packages/test-dev-server/tests/vitest-example/hello-world.browser.test.ts

packages/test-dev-server/tests/vitest.browser.config.ts

knip.jsonc

Copilot

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Files not reviewed (1)

pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

packages/test-dev-server/tests/vitest-setup-playwright.ts

graphite-app · 2025-12-05T10:16:25Z

packages/test-dev-server/src/utils/create-dev-server-plugin.ts

+          source: `
+<h1>HMR Full Bundle Mode</h1>
+
+<div class="app"></div>
+<div class="hmr"></div>
+
+<script type="module" src="./main.js"></script>
  `,


HTML source is missing DOCTYPE declaration and proper HTML structure (html, head, body tags). This generates invalid HTML that may cause unexpected browser rendering behavior and compatibility issues.

source: `<!DOCTYPE html> <html lang="en"> <head> <meta charset="UTF-8"> <meta name="viewport" content="width=device-width, initial-scale=1.0"> <title>HMR Full Bundle Mode</title> </head> <body> <h1>HMR Full Bundle Mode</h1> <div class="app"></div> <div class="hmr"></div> <script type="module" src="./main.js"></script> </body> </html>`

Suggested change

source: `

<h1>HMR Full Bundle Mode</h1>

<div class="app"></div>

<div class="hmr"></div>

<script type="module" src="./main.js"></script>

`,

source: `<!DOCTYPE html>

<html lang="en">

<head>

<meta charset="UTF-8">

<meta name="viewport" content="width=device-width, initial-scale=1.0">

<title>HMR Full Bundle Mode</title>

</head>

<body>

<h1>HMR Full Bundle Mode</h1>

<div class="app"></div>

<div class="hmr"></div>

<script type="module" src="./main.js"></script>

</body>

</html>`,

Spotted by Graphite Agent

Is this helpful? React 👍 or 👎 to let us know.

Copilot

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 13 comments.

Files not reviewed (1)

pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T10:18:07Z

packages/test-dev-server/tests/hmr-full-bundle-mode.spec.ts

+        "const foo = 'hello'",
+        "const foo = 'hello1'",
+      ));
+
+    await expect.poll(() => page.textContent('.hmr')).toBe('hello1');
+
+    editFile(
+      'hmr.js',
+      (code) => code.replace("const foo = 'hello1'", "const foo = 'hello2'"),
+    );
+
+    await expect.poll(() => page.textContent('.hmr')).toBe('hello2');
+
+    editFile('hmr.js', (code) =>
+      code.replace(
+        "const foo = 'hello2'",
+        "const foo = 'hello'",


The string replacement pattern is incorrect. The actual code in hmr.js uses export const foo = 'hello2', not const foo = 'hello2'. The replacement will fail to find a match.

Update the pattern to match the actual code:

editFile('hmr.js', (code) => code.replace( "export const foo = 'hello2'", "export const foo = 'hello'", ));

Suggested change

"const foo = 'hello'",

"const foo = 'hello1'",

));

await expect.poll(() => page.textContent('.hmr')).toBe('hello1');

editFile(

'hmr.js',

(code) => code.replace("const foo = 'hello1'", "const foo = 'hello2'"),

);

await expect.poll(() => page.textContent('.hmr')).toBe('hello2');

editFile('hmr.js', (code) =>

code.replace(

"const foo = 'hello2'",

"const foo = 'hello'",

"export const foo = 'hello'",

"export const foo = 'hello1'",

));

await expect.poll(() => page.textContent('.hmr')).toBe('hello1');

editFile(

'hmr.js',

(code) => code.replace("export const foo = 'hello1'", "export const foo = 'hello2'"),

);

await expect.poll(() => page.textContent('.hmr')).toBe('hello2');

editFile('hmr.js', (code) =>

code.replace(

"export const foo = 'hello2'",

"export const foo = 'hello'",

Copilot · 2025-12-05T10:18:07Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+  await new Promise<void>((resolve) =>
+    setTimeout(() => {
+      resolve();
+    }, 3000)
+  );


Using a fixed 3-second delay to wait for the dev server is unreliable and can lead to flaky tests. If the server takes longer to start (e.g., on slower CI machines), the tests will fail. If it starts faster, the tests will waste time.

Consider polling for server readiness instead:

// Wait for server to be ready by polling the endpoint const maxAttempts = 30; for (let i = 0; i < maxAttempts; i++) { try { const response = await fetch('http://localhost:3000'); if (response.ok) { break; } } catch { // Server not ready yet } await new Promise(resolve => setTimeout(resolve, 1000)); } console.log('[startDevServer] Dev server started.');

Copilot · 2025-12-05T10:18:08Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+
+  console.log('[beforeAll] Launching browser...');
+  browser = await chromium.launch({
+    headless: false, // Can be controlled via env var


The headless: false option will cause tests to fail in CI environments where there's no display available. This should be controlled by an environment variable or set to true by default for CI compatibility.

Update to:

browser = await chromium.launch({ headless: process.env.HEADLESS !== 'false', // Allow running with UI locally via HEADLESS=false });

Suggested change

headless: false, // Can be controlled via env var

headless: process.env.HEADLESS !== 'false', // Allow running with UI locally via HEADLESS=false

Copilot · 2025-12-05T10:18:08Z

packages/test-dev-server/tests/vitest-global-setup.ts

+export async function setup(): Promise<void> {
+  // console.log('Setting up test fixtures in tmp directory...');
+
+  // // Clean up old tmp directory
+  // await rm(CONFIG.paths.tmpFullBundleModeDir, { recursive: true, force: true });
+
+  // // Create fresh tmp directory
+  // await mkdir(CONFIG.paths.tmpFullBundleModeDir, { recursive: true });
+
+  // // Copy hmr-full-bundle-mode to tmp
+
+  // await cp(
+  //   CONFIG.paths.hmrFullBundleModeDir,
+  //   CONFIG.paths.tmpFullBundleModeDir,
+  //   {
+  //     recursive: true,
+  //     dereference: false,
+  //     filter(file) {
+  //       // Exclude .spec.ts test files (only copy source files)
+  //       return !file.endsWith('.spec.ts');
+  //     },
+  //   },
+  // );
+
+  // console.log(`Copied hmr-full-bundle-mode → tmp/hmr-full-bundle-mode`);
+}
+
+/**
+ * Global teardown: Clean up tmp directory
+ */
+export async function teardown(): Promise<void> {
+  // console.log('Cleaning up tmp directory...');
+
+  // // Allow preserving tmp for debugging
+  // if (!process.env.PRESERVE_TMP) {
+  //   await rm(tmpDir, { recursive: true, force: true });
+  //   console.log('Deleted tmp directory');
+  // } else {
+  //   console.log('Preserved tmp directory (PRESERVE_TMP is set)');
+  // }
+}


The entire global setup and teardown functions are commented out, making them non-functional. If this code is not needed, it should be removed rather than left as commented code. If it's intended for future use, add a TODO comment explaining why it's commented out and when it will be activated.

Copilot · 2025-12-05T10:18:08Z

.github/workflows/reusable-node-test.yml

+      - name: Install Playwright
+        # does not need to explicitly set chromium after https://github.com/microsoft/playwright/issues/14862 is solved
+        run: pnpm playwright install chromium


Installing all of Playwright for just Chromium is inefficient and will slow down CI runs. The comment mentions that explicit browser specification isn't needed after an issue is solved, but it's better to install only what's needed now.

Update to install only Chromium:

- name: Install Playwright Chromium run: pnpm exec playwright install --with-deps chromium

The --with-deps flag ensures system dependencies are also installed, which is important for CI environments.

Suggested change

- name: Install Playwright

# does not need to explicitly set chromium after https://github.com/microsoft/playwright/issues/14862 is solved

run: pnpm playwright install chromium

- name: Install Playwright Chromium

# Install only Chromium with system dependencies for CI efficiency

run: pnpm exec playwright install --with-deps chromium

Copilot · 2025-12-05T10:18:10Z

packages/test-dev-server/tests/hmr-full-bundle-mode.spec.ts

+        "const foo = 'hello'",
+        "const foo = 'hello1'",
+      ));
+
+    await expect.poll(() => page.textContent('.hmr')).toBe('hello1');
+
+    editFile(
+      'hmr.js',
+      (code) => code.replace("const foo = 'hello1'", "const foo = 'hello2'"),
+    );
+
+    await expect.poll(() => page.textContent('.hmr')).toBe('hello2');
+
+    editFile('hmr.js', (code) =>
+      code.replace(
+        "const foo = 'hello2'",
+        "const foo = 'hello'",


The string replacement pattern is incorrect. The actual code in hmr.js uses export const foo = 'hello', not const foo = 'hello'. The replacement will fail to find a match and the test will not work as expected.

Update the pattern to match the actual code:

editFile('hmr.js', (code) => code.replace( "export const foo = 'hello'", "export const foo = 'hello1'", ));

Suggested change

"const foo = 'hello'",

"const foo = 'hello1'",

));

await expect.poll(() => page.textContent('.hmr')).toBe('hello1');

editFile(

'hmr.js',

(code) => code.replace("const foo = 'hello1'", "const foo = 'hello2'"),

);

await expect.poll(() => page.textContent('.hmr')).toBe('hello2');

editFile('hmr.js', (code) =>

code.replace(

"const foo = 'hello2'",

"const foo = 'hello'",

"export const foo = 'hello'",

"export const foo = 'hello1'",

));

await expect.poll(() => page.textContent('.hmr')).toBe('hello1');

editFile(

'hmr.js',

(code) => code.replace("export const foo = 'hello1'", "export const foo = 'hello2'"),

);

await expect.poll(() => page.textContent('.hmr')).toBe('hello2');

editFile('hmr.js', (code) =>

code.replace(

"export const foo = 'hello2'",

"export const foo = 'hello'",

Copilot · 2025-12-05T10:18:10Z

packages/test-dev-server/tests/hmr-full-bundle-mode.spec.ts

+        "const foo = 'hello'",
+        "const foo = 'hello1'",
+      ));
+
+    await expect.poll(() => page.textContent('.hmr')).toBe('hello1');
+
+    editFile(
+      'hmr.js',
+      (code) => code.replace("const foo = 'hello1'", "const foo = 'hello2'"),
+    );
+
+    await expect.poll(() => page.textContent('.hmr')).toBe('hello2');
+
+    editFile('hmr.js', (code) =>
+      code.replace(
+        "const foo = 'hello2'",
+        "const foo = 'hello'",


The string replacement pattern is incorrect. The actual code in hmr.js uses export const foo = 'hello1', not const foo = 'hello1'. The replacement will fail to find a match.

Update the pattern to match the actual code:

editFile( 'hmr.js', (code) => code.replace("export const foo = 'hello1'", "export const foo = 'hello2'"), );

Suggested change

"const foo = 'hello'",

"const foo = 'hello1'",

));

await expect.poll(() => page.textContent('.hmr')).toBe('hello1');

editFile(

'hmr.js',

(code) => code.replace("const foo = 'hello1'", "const foo = 'hello2'"),

);

await expect.poll(() => page.textContent('.hmr')).toBe('hello2');

editFile('hmr.js', (code) =>

code.replace(

"const foo = 'hello2'",

"const foo = 'hello'",

"export const foo = 'hello'",

"export const foo = 'hello1'",

));

await expect.poll(() => page.textContent('.hmr')).toBe('hello1');

editFile(

'hmr.js',

(code) => code.replace("export const foo = 'hello1'", "export const foo = 'hello2'"),

);

await expect.poll(() => page.textContent('.hmr')).toBe('hello2');

editFile('hmr.js', (code) =>

code.replace(

"export const foo = 'hello2'",

"export const foo = 'hello'",

Copilot · 2025-12-05T10:18:10Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+  // @ts-expect-error
+  ({ devServerProcess } = await startDevServer());


The type assertion @ts-expect-error is used to suppress TypeScript errors, but the destructuring assignment appears problematic. The devServerProcess is being assigned from a destructured object, but the function returns { devServerProcess }, so this creates nested references.

The code should be:

devServerProcess = (await startDevServer()).devServerProcess;

Or better yet, update startDevServer() to return just the process directly instead of wrapping it in an object.

Suggested change

// @ts-expect-error

({ devServerProcess } = await startDevServer());

devServerProcess = (await startDevServer()).devServerProcess;

Copilot · 2025-12-05T10:18:11Z

packages/test-dev-server/src/utils/create-dev-server-plugin.ts

+          source: `
+<h1>HMR Full Bundle Mode</h1>
+
+<div class="app"></div>
+<div class="hmr"></div>
+
+<script type="module" src="./main.js"></script>
  `,


The generated HTML is missing essential HTML structure including <!DOCTYPE html>, <html>, <head>, and <body> tags. This is invalid HTML and may cause unexpected browser behavior.

The HTML should follow proper structure:

source: `<!DOCTYPE html> <html lang="en"> <head> <meta charset="UTF-8" /> <meta name="viewport" content="width=device-width, initial-scale=1.0" /> <title>HMR Full Bundle Mode</title> </head> <body> <h1>HMR Full Bundle Mode</h1> <div class="app"></div> <div class="hmr"></div> <script type="module" src="./main.js"></script> </body> </html>`,

Suggested change

source: `

<h1>HMR Full Bundle Mode</h1>

<div class="app"></div>

<div class="hmr"></div>

<script type="module" src="./main.js"></script>

`,

source: `<!DOCTYPE html>

<html lang="en">

<head>

<meta charset="UTF-8" />

<meta name="viewport" content="width=device-width, initial-scale=1.0" />

<title>HMR Full Bundle Mode</title>

</head>

<body>

<h1>HMR Full Bundle Mode</h1>

<div class="app"></div>

<div class="hmr"></div>

<script type="module" src="./main.js"></script>

</body>

</html>

`,

packages/test-dev-server/tests/src/config.ts

Copilot

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 12 comments.

Files not reviewed (1)

pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T15:06:03Z

package.json

    "oxlint-tsgolint": "0.8.3",
    "remove-unused-vars": "^0.0.10",
    "rolldown": "workspace:*",
+    "playwright-chromium": "^1.56.1",


There's a version mismatch between playwright dependencies:

Root package.json specifies playwright-chromium@^1.56.1

packages/test-dev-server/tests/package.json specifies playwright@^1.48.0

pnpm-lock.yaml shows resolved versions of 1.57.0

The versions should be aligned. Consider using the same version range across all playwright-related packages to avoid compatibility issues.

Suggested change

"playwright-chromium": "^1.56.1",

"playwright-chromium": "^1.57.0",

Copilot · 2025-12-05T15:06:03Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+  await page.goto('http://localhost:3000', { waitUntil: 'networkidle' });
+
+  // Make page available to tests
+  (global as any).__page = page;


Using (global as any) bypasses TypeScript's type safety. Consider defining a proper type for the global object:

declare global { var __page: Page | undefined; }

Then you can use global.__page with proper type checking.

Copilot · 2025-12-05T15:06:04Z

packages/test-dev-server/tests/playground/hmr-full-bundle-mode/dev.config.mjs

+      waitBundleCompleteUntilAccess(),
+      delayTransformComment(),
+    ],
+  },
+});
+
+/**
+ * Plugin: Wait for first server access before completing bundle
+ * Simulates bundle completion timing for testing
+ */
+function waitBundleCompleteUntilAccess() {
+  return {
+    name: 'wait-bundle-complete-until-access',


The function name waitBundleCompleteUntilAccess is misleading. The function doesn't actually wait for server access - it just delays bundle generation by 300ms unconditionally.

Consider renaming to something more accurate like delayBundleGeneration or update the implementation to actually wait for server access if that's the intended behavior.

Suggested change

waitBundleCompleteUntilAccess(),

delayTransformComment(),

],

},

});

/**

* Plugin: Wait for first server access before completing bundle

* Simulates bundle completion timing for testing

*/

function waitBundleCompleteUntilAccess() {

return {

name: 'wait-bundle-complete-until-access',

delayBundleGeneration(),

delayTransformComment(),

],

},

});

/**

* Plugin: Delay bundle generation by 300ms

* Simulates bundle completion timing for testing

*/

function delayBundleGeneration() {

return {

name: 'delay-bundle-generation',

Copilot · 2025-12-05T15:06:04Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+  if (devServerProcess) {
+    devServerProcess.kill('SIGTERM');
+    devServerProcess = null;
+  }


The cleanup in afterAll doesn't remove the temporary playground directory that was created in beforeAll. This could lead to disk space issues over time or interfere with subsequent test runs.

Consider adding cleanup for the tmp directory:

// Clean up tmp playground directory await nodeFs.promises.rm(CONFIG.paths.tmpPlaygroundDir, { recursive: true, force: true, });

Suggested change

}

}

// Clean up tmp playground directory

await nodeFs.promises.rm(CONFIG.paths.tmpPlaygroundDir, {

recursive: true,

force: true,

});

Copilot · 2025-12-05T15:06:04Z

packages/test-dev-server/tests/vitest.config.browser.mts

+
+export default defineConfig({
+  test: {
+    hookTimeout: 1000 * 30,


The hookTimeout is set to 30 seconds (30000ms) which may not be sufficient for slow CI environments, especially given that the testTimeout is 90 seconds. If beforeAll needs to start the dev server and launch Playwright, it could easily exceed 30 seconds in CI.

Consider increasing it to match or be proportional to testTimeout:

hookTimeout: 60000, // 60 seconds for hooks in CI

Suggested change

hookTimeout: 1000 * 30,

hookTimeout: 1000 * 60, // 60 seconds for hooks in CI

Copilot · 2025-12-05T15:06:05Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+
+async function startDevServer() {
+  console.log('[startDevServer] Starting dev server...');
+  const devServerProcess = execa('pnpm serve', {


The command pnpm serve doesn't match any script in the package.json. The package.json defines "dev": "serve", so the command should be pnpm dev instead.

Change to:

const devServerProcess = execa('pnpm dev', {

Suggested change

const devServerProcess = execa('pnpm serve', {

const devServerProcess = execa('pnpm dev', {

Copilot · 2025-12-05T15:06:05Z

packages/test-dev-server/tests/vitest-global-setup.ts

+/**
+ * Global setup: Copy test fixtures to tmp directory
+ */
+export async function setup(): Promise<void> {
+  // console.log('Setting up test fixtures in tmp directory...');
+
+  // // Clean up old tmp directory
+  // await rm(CONFIG.paths.tmpFullBundleModeDir, { recursive: true, force: true });
+
+  // // Create fresh tmp directory
+  // await mkdir(CONFIG.paths.tmpFullBundleModeDir, { recursive: true });
+
+  // // Copy hmr-full-bundle-mode to tmp
+
+  // await cp(
+  //   CONFIG.paths.hmrFullBundleModeDir,
+  //   CONFIG.paths.tmpFullBundleModeDir,
+  //   {
+  //     recursive: true,
+  //     dereference: false,
+  //     filter(file) {
+  //       // Exclude .spec.ts test files (only copy source files)
+  //       return !file.endsWith('.spec.ts');
+  //     },
+  //   },
+  // );
+
+  // console.log(`Copied hmr-full-bundle-mode → tmp/hmr-full-bundle-mode`);
+}
+
+/**
+ * Global teardown: Clean up tmp directory
+ */
+export async function teardown(): Promise<void> {
+  // console.log('Cleaning up tmp directory...');
+
+  // // Allow preserving tmp for debugging
+  // if (!process.env.PRESERVE_TMP) {
+  //   await rm(tmpDir, { recursive: true, force: true });
+  //   console.log('Deleted tmp directory');
+  // } else {
+  //   console.log('Preserved tmp directory (PRESERVE_TMP is set)');
+  // }


The global setup file exports setup and teardown functions but they are entirely commented out and do nothing. This creates dead code and confusion.

Consider either implementing the functions properly or removing the file entirely if it's not needed.

Suggested change

/**

* Global setup: Copy test fixtures to tmp directory

*/

export async function setup(): Promise<void> {

// console.log('Setting up test fixtures in tmp directory...');

// // Clean up old tmp directory

// await rm(CONFIG.paths.tmpFullBundleModeDir, { recursive: true, force: true });

// // Create fresh tmp directory

// await mkdir(CONFIG.paths.tmpFullBundleModeDir, { recursive: true });

// // Copy hmr-full-bundle-mode to tmp

// await cp(

// CONFIG.paths.hmrFullBundleModeDir,

// CONFIG.paths.tmpFullBundleModeDir,

// {

// recursive: true,

// dereference: false,

// filter(file) {

// // Exclude .spec.ts test files (only copy source files)

// return !file.endsWith('.spec.ts');

// },

// },

// );

// console.log(`Copied hmr-full-bundle-mode → tmp/hmr-full-bundle-mode`);

}

/**

* Global teardown: Clean up tmp directory

*/

export async function teardown(): Promise<void> {

// console.log('Cleaning up tmp directory...');

// // Allow preserving tmp for debugging

// if (!process.env.PRESERVE_TMP) {

// await rm(tmpDir, { recursive: true, force: true });

// console.log('Deleted tmp directory');

// } else {

// console.log('Preserved tmp directory (PRESERVE_TMP is set)');

// }

import { rm, mkdir, cp } from 'fs/promises';

import * as path from 'path';

// Minimal CONFIG object for demonstration; replace with actual config as needed

const CONFIG = {

paths: {

hmrFullBundleModeDir: path.resolve(__dirname, 'fixtures/hmr-full-bundle-mode'),

tmpFullBundleModeDir: path.resolve(__dirname, 'tmp/hmr-full-bundle-mode'),

},

};

const tmpDir = path.resolve(__dirname, 'tmp');

/**

* Global setup: Copy test fixtures to tmp directory

*/

export async function setup(): Promise<void> {

console.log('Setting up test fixtures in tmp directory...');

// Clean up old tmp directory

await rm(CONFIG.paths.tmpFullBundleModeDir, { recursive: true, force: true }).catch(() => {});

// Create fresh tmp directory

await mkdir(CONFIG.paths.tmpFullBundleModeDir, { recursive: true });

// Copy hmr-full-bundle-mode to tmp, excluding .spec.ts files

await cp(

CONFIG.paths.hmrFullBundleModeDir,

CONFIG.paths.tmpFullBundleModeDir,

{

recursive: true,

dereference: false,

filter(file) {

// Exclude .spec.ts test files (only copy source files)

return !file.endsWith('.spec.ts');

},

},

);

console.log(`Copied hmr-full-bundle-mode → tmp/hmr-full-bundle-mode`);

}

/**

* Global teardown: Clean up tmp directory

*/

export async function teardown(): Promise<void> {

console.log('Cleaning up tmp directory...');

// Allow preserving tmp for debugging

if (!process.env.PRESERVE_TMP) {

await rm(tmpDir, { recursive: true, force: true }).catch(() => {});

console.log('Deleted tmp directory');

} else {

console.log('Preserved tmp directory (PRESERVE_TMP is set)');

}

Copilot · 2025-12-05T15:06:06Z

packages/test-dev-server/tests/test-utils.ts

@@ -0,0 +1,39 @@
+import nodeFs from 'node:fs';
+import { resolve } from 'node:path';
+import { CONFIG } from './src/config.js';


[nitpick] The import uses .js extension for a .ts file. While this may work with certain TypeScript configurations, it's inconsistent with other imports in the codebase that don't use extensions.

Consider removing the .js extension:

import { CONFIG } from './src/config';

Suggested change

import { CONFIG } from './src/config.js';

import { CONFIG } from './src/config';

Copilot · 2025-12-05T15:06:06Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+        '[startDevServer] Dev server process terminated with SIGTERM.',
+      );


[nitpick] The catch block for ExecaError with SIGTERM signal suppresses the error silently with only a console.log. This could make debugging difficult if the dev server terminates unexpectedly.

Consider logging more context or re-throwing in non-test cleanup scenarios:

console.log( '[startDevServer] Dev server process terminated with SIGTERM (this is expected during cleanup).', ); return null; // Indicate graceful shutdown

Suggested change

'[startDevServer] Dev server process terminated with SIGTERM.',

);

`[startDevServer] Dev server process (pid: ${err.pid}) terminated with SIGTERM (cwd: ${CONFIG.paths.tmpFullBundleModeDir}). This is expected during test cleanup.`

);

// If not in test cleanup, re-throw for debugging

if (!process.env.VITEST_WORKER_ID) {

throw err;

}

Copilot · 2025-12-05T15:06:06Z

packages/test-dev-server/tests/test-utils.ts

+ * Get the Playwright page from global context
+ */
+export function getPage() {
+  const page = (global as any).__page;


Using (global as any) bypasses TypeScript's type safety. This is the same issue as in vitest-setup-playwright.ts. Consider defining a proper global type declaration.

graphite-app · 2025-12-05T16:01:54Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+
+async function startDevServer() {
+  console.log('[startDevServer] Starting dev server...');
+  const subprocess = execa('pnpm serve', {


Critical bug: The command pnpm serve will fail because the package.json defines the script as dev, not serve.

Looking at packages/test-dev-server/tests/playground/hmr-full-bundle-mode/package.json, the script is:

"scripts": { "dev": "serve" }

The code attempts to run pnpm serve but should run pnpm dev instead:

const subprocess = execa('pnpm dev', { cwd: CONFIG.paths.tmpFullBundleModeDir, // ... });

This will cause the dev server to fail to start, causing all browser tests to fail.

Suggested change

const subprocess = execa('pnpm serve', {

const subprocess = execa('pnpm dev', {

Spotted by Graphite Agent

Is this helpful? React 👍 or 👎 to let us know.

Copilot

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 8 comments.

Files not reviewed (1)

pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T17:15:03Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+  for (let i = 0; i < maxAttempts; i++) {
+    try {
+      const response = await fetch('http://localhost:3000');
+      if (response.ok) {
+        return;
+      }
+    } catch {}
+    await new Promise(r => setTimeout(r, 200));
+  }
+  throw new Error('Server failed to start');


The error message "Server failed to start" is not descriptive enough. It doesn't indicate how long the function waited or what the actual issue might be. Consider including the timeout duration and the URL being checked in the error message:

throw new Error(`Server failed to start at http://localhost:3000 after ${maxAttempts * 200}ms`);

Suggested change

for (let i = 0; i < maxAttempts; i++) {

try {

const response = await fetch('http://localhost:3000');

if (response.ok) {

return;

}

} catch {}

await new Promise(r => setTimeout(r, 200));

}

throw new Error('Server failed to start');

const url = 'http://localhost:3000';

const delayMs = 200;

for (let i = 0; i < maxAttempts; i++) {

try {

const response = await fetch(url);

if (response.ok) {

return;

}

} catch {}

await new Promise(r => setTimeout(r, delayMs));

}

throw new Error(

`Server failed to start at ${url} after ${maxAttempts * delayMs}ms (${maxAttempts} attempts)`

);

Copilot · 2025-12-05T17:15:04Z

packages/test-dev-server/tests/hmr-full-bundle-mode.spec.ts

@@ -0,0 +1,43 @@
+import { describe, expect, test } from 'vitest';


The new test file uses the .spec.ts naming convention, which is inconsistent with the existing .test.ts pattern used in this codebase (see fixtures.test.ts). For consistency, this file should be named hmr-full-bundle-mode.test.ts instead of hmr-full-bundle-mode.spec.ts.

Copilot · 2025-12-05T17:15:04Z

packages/test-dev-server/tests/test-utils.ts

@@ -0,0 +1,50 @@
+import nodeFs from 'node:fs';
+import { resolve } from 'node:path';
+import { CONFIG } from './src/config.js';


The import uses .js extension ('./src/config.js') but the actual file is config.ts. While this works in some TypeScript configurations that resolve .js to .ts files, it's inconsistent with the other imports in the file (e.g., line 2 doesn't use an extension). For consistency and clarity, either:

Use './src/config' without extension (recommended for consistency)

Use './src/config.ts' to match the actual file

Suggested change

import { CONFIG } from './src/config.js';

import { CONFIG } from './src/config';

Copilot · 2025-12-05T17:15:04Z

packages/test-dev-server/tests/vitest.config.browser.mts

+export default defineConfig({
+  test: {
+    hookTimeout: 1000 * 30,
+    // Include Node.js test files (*.spec.ts, not *.browser.test.ts)


The comment states "Include Node.js test files (*.spec.ts, not *.browser.test.ts)" but this is confusing because:

The include pattern is **/*.spec.{js,ts} which only matches .spec files

There are no .browser.test.ts files in this PR

The comment implies exclusion of browser test files, but the pattern doesn't actually exclude anything

Consider clarifying the comment to simply state what is being included: "Include Playwright E2E test files (*.spec.ts)" or remove the misleading reference to .browser.test.ts files.

Suggested change

// Include Node.js test files (*.spec.ts, not *.browser.test.ts)

// Include test files (*.spec.js, *.spec.ts)

Copilot · 2025-12-05T17:15:05Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+  const createTmpPlaygroundDirPromise = createTmpPlaygroundDir();
+  await killPort(3000);
+  await createTmpPlaygroundDirPromise;


[nitpick] The beforeAll setup has a potential race condition. If killPort(3000) completes before createTmpPlaygroundDir() finishes and the dev server starts immediately after, it might try to start the server before the playground files are fully copied. While this seems unlikely due to the await ordering, it's clearer to await createTmpPlaygroundDir() first, then killPort(). This makes the sequential dependency explicit:

await createTmpPlaygroundDir(); await killPort(3000); ({ devServerProcess } = await startDevServer());

Suggested change

const createTmpPlaygroundDirPromise = createTmpPlaygroundDir();

await killPort(3000);

await createTmpPlaygroundDirPromise;

await createTmpPlaygroundDir();

await killPort(3000);

Copilot · 2025-12-05T17:15:05Z

packages/test-dev-server/tests/test-utils.ts

+  if (content === newContent) {
+    console.warn(`[editFile] No changes detected for ${filename}`);
+    return;
+  }


[nitpick] The console.warn message when no changes are detected could be misleading in tests where the replacer function is expected to sometimes not find a match (e.g., if checking for a specific state). Consider either making this a debug-level log or removing it, as the caller can check if they need to know whether changes occurred.

Suggested change

if (content === newContent) {

console.warn(`[editFile] No changes detected for ${filename}`);

return;

}

// No changes detected; proceed without warning or early return

Copilot · 2025-12-05T17:15:05Z

packages/test-dev-server/tests/package.json

  "license": "MIT",
  "scripts": {
-    "test": "vitest run"
+    "test": "pnpm run test:legacy && pnpm run test:browser",


[nitpick] The test script runs both test:legacy and test:browser sequentially. If either test suite can run independently, consider using pnpm run -p (parallel execution) instead for faster CI times. However, if they share resources (like ports), sequential execution is correct. Add a comment explaining why sequential execution is necessary if that's the case.

Suggested change

"test": "pnpm run test:legacy && pnpm run test:browser",

"test": "pnpm run -p test:legacy test:browser",

Copilot · 2025-12-05T17:15:06Z

pnpm-workspace.yaml

  - [email protected] # https://github.com/paulmillr/chokidar/issues/1440
  - [email protected]
-  - [email protected]
+  - [email protected] # Trust downgrade when installing @vitest/browser-playwright


The comment "Trust downgrade when installing @vitest/browser-playwright" should explain why this trust downgrade is necessary. What specific issue does it resolve? Consider adding more context, similar to the chokidar entry that references a specific GitHub issue.

Suggested change

- [email protected] # Trust downgrade when installing @vitest/browser-playwright

- [email protected] # Trust downgrade required due to @vitest/browser-playwright's dependency on [email protected], which triggers pnpm trust policy issues. See https://github.com/vitest-dev/vitest/issues/5372

graphite-app · 2025-12-05T18:36:23Z

packages/test-dev-server/tests/vitest-setup-playwright.ts

+  if (devServerProcess) {
+    devServerProcess.kill('SIGTERM');
+    devServerProcess = null;
+  }


Race condition: Dev server may not terminate before process cleanup

Calling kill('SIGTERM') does not wait for the process to actually terminate. The afterAll hook will complete immediately while the dev server may still be running, potentially causing port conflicts in subsequent test runs or CI environments.

// Fix: Wait for process to exit if (devServerProcess) { devServerProcess.kill('SIGTERM'); await devServerProcess.catch(() => {}); // Wait for termination devServerProcess = null; }

Suggested change

if (devServerProcess) {

devServerProcess.kill('SIGTERM');

devServerProcess = null;

}

if (devServerProcess) {

devServerProcess.kill('SIGTERM');

await devServerProcess.catch(() => {}); // Wait for termination

devServerProcess = null;

}

Spotted by Graphite Agent

Is this helpful? React 👍 or 👎 to let us know.

#7325 is too messy, so I re-create this one. I left todos at #7350

github-actions bot assigned hyf0 Dec 3, 2025

hyf0 marked this pull request as ready for review December 3, 2025 07:00

Copilot AI review requested due to automatic review settings December 3, 2025 07:00

Copilot started reviewing on behalf of hyf0 December 3, 2025 07:01 View session

graphite-app bot reviewed Dec 3, 2025

View reviewed changes

packages/test-dev-server/tests/package.json Outdated Show resolved Hide resolved

Copilot finished reviewing on behalf of hyf0 December 3, 2025 07:03

Copilot AI reviewed Dec 3, 2025

View reviewed changes

graphite-app bot reviewed Dec 3, 2025

View reviewed changes

packages/test-dev-server/tests/package.json Outdated Show resolved Hide resolved

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from bb5240e to d8346fe Compare December 3, 2025 07:51

Copilot AI review requested due to automatic review settings December 3, 2025 08:10

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from d8346fe to 0f15ff2 Compare December 3, 2025 08:10

Copilot started reviewing on behalf of hyf0 December 3, 2025 08:10 View session

Copilot finished reviewing on behalf of hyf0 December 3, 2025 08:12

Copilot AI reviewed Dec 3, 2025

View reviewed changes

packages/test-dev-server/tests/vitest-example/hello-world.browser.test.ts Outdated Show resolved Hide resolved

packages/test-dev-server/tests/vitest.browser.config.ts Outdated Show resolved Hide resolved

knip.jsonc Outdated Show resolved Hide resolved

Copilot AI review requested due to automatic review settings December 3, 2025 09:17

Copilot started reviewing on behalf of hyf0 December 3, 2025 09:17 View session

Copilot finished reviewing on behalf of hyf0 December 3, 2025 09:20

Copilot AI reviewed Dec 3, 2025

View reviewed changes

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from aacdaaa to 12163ca Compare December 3, 2025 18:57

hyf0 changed the title ~~chore(repo): setup vitest browser mode for test-dev-server~~ chore(repo): setup browser-based e2e test for test-dev-server Dec 3, 2025

hyf0 changed the title ~~chore(repo): setup browser-based e2e test for test-dev-server~~ chore(test): setup browser-based e2e test for test-dev-server Dec 3, 2025

graphite-app bot reviewed Dec 3, 2025

View reviewed changes

packages/test-dev-server/tests/vitest-setup-playwright.ts Outdated Show resolved Hide resolved

packages/test-dev-server/tests/vitest-setup-playwright.ts Outdated Show resolved Hide resolved

Copilot AI review requested due to automatic review settings December 5, 2025 10:11

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from 12163ca to 4ce769e Compare December 5, 2025 10:11

Copilot started reviewing on behalf of hyf0 December 5, 2025 10:12 View session

Copilot finished reviewing on behalf of hyf0 December 5, 2025 10:14

graphite-app bot reviewed Dec 5, 2025

View reviewed changes

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from 4ce769e to 85708df Compare December 5, 2025 10:17

Copilot AI reviewed Dec 5, 2025

View reviewed changes

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from 85708df to b96ea91 Compare December 5, 2025 10:24

Copilot AI review requested due to automatic review settings December 5, 2025 14:57

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from b96ea91 to 95eec27 Compare December 5, 2025 14:57

Copilot started reviewing on behalf of hyf0 December 5, 2025 14:58 View session

Copilot finished reviewing on behalf of hyf0 December 5, 2025 15:02

Copilot AI reviewed Dec 5, 2025

View reviewed changes

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from 95eec27 to f79a4c6 Compare December 5, 2025 15:58

graphite-app bot reviewed Dec 5, 2025

View reviewed changes

hyf0 assigned sapphi-red Dec 5, 2025

Copilot AI review requested due to automatic review settings December 5, 2025 17:08

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from f79a4c6 to b4091dc Compare December 5, 2025 17:08

Copilot started reviewing on behalf of hyf0 December 5, 2025 17:08 View session

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from b4091dc to bde6ce1 Compare December 5, 2025 17:09

Copilot finished reviewing on behalf of hyf0 December 5, 2025 17:12

Copilot AI reviewed Dec 5, 2025

View reviewed changes

chore(repo): setup vitest browser mode for test-dev-server

a9e4e6d

hyf0 mentioned this pull request Dec 5, 2025

Improve test-dev-server and browser-based test infra #7350

Open

5 tasks

hyf0 force-pushed the 12-03-chore_repo_setup_vitest_browser_mode_for_test-dev-server_ branch from bde6ce1 to a9e4e6d Compare December 5, 2025 18:32

graphite-app bot reviewed Dec 5, 2025

View reviewed changes

hyf0 closed this Dec 6, 2025

hyf0 mentioned this pull request Dec 6, 2025

chore(test): setup browser-based e2e test for test-dev-server #7351

Merged

graphite-app bot pushed a commit that referenced this pull request Dec 8, 2025

chore(test): setup browser-based e2e test for test-dev-server (#7351)

3ad3391

#7325 is too messy, so I re-create this one. I left todos at #7350

	headless: false, // Can be controlled via env var
	headless: process.env.HEADLESS !== 'false', // Allow running with UI locally via HEADLESS=false

		// @ts-expect-error
		({ devServerProcess } = await startDevServer());

	// @ts-expect-error
	({ devServerProcess } = await startDevServer());
	devServerProcess = (await startDevServer()).devServerProcess;

	"playwright-chromium": "^1.56.1",
	"playwright-chromium": "^1.57.0",

-  }
+  }
+  // Clean up tmp playground directory
+  await nodeFs.promises.rm(CONFIG.paths.tmpPlaygroundDir, {
+    recursive: true,
+    force: true,
+  });

	hookTimeout: 1000 * 30,
	hookTimeout: 1000 * 60, // 60 seconds for hooks in CI

	const devServerProcess = execa('pnpm serve', {
	const devServerProcess = execa('pnpm dev', {

	import { CONFIG } from './src/config.js';
	import { CONFIG } from './src/config';

		'[startDevServer] Dev server process terminated with SIGTERM.',
		);

-        '[startDevServer] Dev server process terminated with SIGTERM.',
-      );
+        `[startDevServer] Dev server process (pid: ${err.pid}) terminated with SIGTERM (cwd: ${CONFIG.paths.tmpFullBundleModeDir}). This is expected during test cleanup.`
+      );
+      // If not in test cleanup, re-throw for debugging
+      if (!process.env.VITEST_WORKER_ID) {
+        throw err;
+      }

	const subprocess = execa('pnpm serve', {
	const subprocess = execa('pnpm dev', {

		@@ -0,0 +1,43 @@
		import { describe, expect, test } from 'vitest';

	// Include Node.js test files (.spec.ts, not .browser.test.ts)
	// Include test files (.spec.js, .spec.ts)

	"test": "pnpm run test:legacy && pnpm run test:browser",
	"test": "pnpm run -p test:legacy test:browser",

	- [email protected] # Trust downgrade when installing @vitest/browser-playwright
	- [email protected] # Trust downgrade required due to @vitest/browser-playwright's dependency on [email protected], which triggers pnpm trust policy issues. See https://github.com/vitest-dev/vitest/issues/5372

Comments

Conversation

hyf0 commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hyf0 commented Dec 3, 2025

How to use the Graphite Merge Queue

Uh oh!

socket-security bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for rolldown-rs canceled.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

graphite-app bot Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

hyf0 commented Dec 3, 2025 •

edited

Loading

socket-security bot commented Dec 3, 2025 •

edited

Loading

netlify bot commented Dec 3, 2025 •

edited

Loading