Skip to content

Commit 557d377

Browse files
Boshenclaude
Boshen
and
claude
authored
test: make dev server tests deterministic by replacing fixed sleeps with event-driven polling (#8561)
## Summary - Add a `/_dev/status` endpoint to the dev server exposing `buildSeq`, `moduleRegistrationSeq`, and bundle state counters - Replace all fixed `setTimeout` sleeps in test synchronization with event-driven polling utilities (`waitForNextBuild`, `waitForBuildStable`, `waitForModuleRegistration`) - Remove `sensibleTimeoutInMs` helper and unused `CONFIG.watch` references - On retry, fully restart the dev server with fresh files instead of relying on sleep + page reload - Fix `buildSeq` double-increment on full-reload HMR builds (skip increment in `onHmrUpdates` when FullReload is present, since `onOutput` will handle it) Eliminates all hardcoded sleeps (710ms-6000ms, x3 on CI) that caused flakiness and wasted CI minutes. ### Local test results (macOS, 3 runs each) | | main (sleeps) | this PR (polling) | Improvement | |---|---|---|---| | **Fixtures** | 17.24s avg | 7.54s avg | **56% faster** | | **Browser** | 10.76s avg | 9.43s avg | **12% faster** | | **Total** | 28.00s avg | 16.97s avg | **39% faster** | | **Reliability** | 3/3 passed | 3/3 passed | OK | ### CI job time comparison (avg of 3 runs each) | Platform | main | this PR | saved | |----------|------|---------|-------| | Ubuntu | 5m28s | 3m36s | 1m52s (34% faster) | | macOS | 5m05s | 2m55s | 2m10s (43% faster) | | Windows | 7m30s | 5m36s | 1m54s (25% faster) | Closes #8556 --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
1 parent 7d89c63 commit 557d377

File tree

7 files changed

+189
-41
lines changed

7 files changed

+189
-41
lines changed

‎packages/test-dev-server/src/dev-server.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ class DevServer {
4646
#devOptions?: NormalizedDevOptions;
4747
#devEngine?: DevEngine;
4848
#port = 3000;
49+
#buildSeq = 0;
50+
#moduleRegistrationSeq = 0;
4951

5052
constructor() {}
5153

@@ -88,14 +90,23 @@ class DevServer {
8890
onHmrUpdates: (errOrUpdates) => {
8991
if (errOrUpdates instanceof Error) {
9092
console.error('HMR update error:', errOrUpdates);
93+
this.#buildSeq++;
9194
} else {
9295
this.handleHmrUpdates(errOrUpdates.updates);
96+
// Only increment if no FullReload — a FullReload triggers a rebuild
97+
// which will call onOutput, so we let onOutput do the increment to
98+
// avoid double-counting a single build cycle.
99+
const hasFullReload = errOrUpdates.updates.some((u) => u.update.type === 'FullReload');
100+
if (!hasFullReload) {
101+
this.#buildSeq++;
102+
}
93103
}
94104
},
95105
onOutput: (errOrOutputs) => {
96106
if (errOrOutputs instanceof Error) {
97107
console.error('Build error:', errOrOutputs);
98108
}
109+
this.#buildSeq++;
99110
},
100111
watch: getDevWatchOptionsForCi(),
101112
});
@@ -169,6 +180,7 @@ class DevServer {
169180
case 'hmr:module-registered': {
170181
console.log('Registering modules:', clientMessage.modules);
171182
this.#devEngine?.registerModules(clientSession.id, clientMessage.modules);
183+
this.#moduleRegistrationSeq++;
172184
break;
173185
}
174186
default: {
@@ -216,6 +228,23 @@ class DevServer {
216228
}
217229
next();
218230
});
231+
this.connectServer.use(async (req, res, next) => {
232+
if (req.url === '/_dev/status') {
233+
const bundleState = await devEngine.getBundleState();
234+
res.setHeader('Content-Type', 'application/json');
235+
res.end(
236+
JSON.stringify({
237+
hasStaleOutput: bundleState.hasStaleOutput,
238+
lastFullBuildFailed: bundleState.lastFullBuildFailed,
239+
buildSeq: this.#buildSeq,
240+
connectedClients: this.#clients.size,
241+
moduleRegistrationSeq: this.#moduleRegistrationSeq,
242+
}),
243+
);
244+
return;
245+
}
246+
next();
247+
});
219248
this.connectServer.use(
220249
serveStatic(nodePath.join(process.cwd(), 'dist'), {
221250
index: ['index.html'],

‎packages/test-dev-server/tests/fixtures.test.ts‎

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,14 @@ import killPort from 'kill-port';
55
import nodeFs from 'node:fs';
66
import nodePath from 'node:path';
77
import { afterAll, describe, test } from 'vitest';
8-
import { CONFIG } from './src/config';
9-
import { isDirectoryExists, removeDirSync, sensibleTimeoutInMs } from './src/utils';
8+
import { isDirectoryExists, removeDirSync } from './src/utils';
9+
import {
10+
getBuildSeq,
11+
getModuleRegistrationSeq,
12+
waitForBuildStable,
13+
waitForModuleRegistration,
14+
waitForNextBuild,
15+
} from './test-utils';
1016

1117
function main() {
1218
const fixturesPath = nodePath.resolve(__dirname, 'fixtures');
@@ -82,11 +88,20 @@ function main() {
8288

8389
await waitForPathExists(nodeScriptPath);
8490

85-
let runningArtifactProcess = await runArtifactProcess(nodeScriptPath, tmpProjectPath);
91+
let runningArtifactProcess = await runArtifactProcess(nodeScriptPath, tmpProjectPath, port);
8692

8793
const hmrEditFiles = await collectHmrEditFiles(tmpProjectPath);
8894

95+
// Wait for the initial build to stabilize
96+
await waitForBuildStable(port);
97+
8998
for (const [index, [step, hmrEdits]] of hmrEditFiles.entries()) {
99+
// Wait for the previous build's debounce window to close so the
100+
// watcher treats the next file write as a new change.
101+
if (index !== 0) {
102+
await waitForBuildStable(port);
103+
}
104+
90105
console.log(
91106
`🔄 Processing HMR edit files for step ${step} with edits: ${JSON.stringify(
92107
hmrEdits,
@@ -95,17 +110,6 @@ function main() {
95110
)}`,
96111
);
97112

98-
// Refer to `packages/test-dev-server/src/utils/get-dev-watch-options-for-ci.ts`
99-
// We used a poll-based and debounced watcher in CI, so we need to wait for certain amount of time to
100-
// - Make sure different steps are not debounced together
101-
// - Make sure changes are detected individually for different steps
102-
// - Make sure changes in the same step are detected together
103-
if (index !== 0) {
104-
await sensibleTimeoutInMs(
105-
CONFIG.watch.debounceDuration + CONFIG.watch.debounceTickRate + 100,
106-
);
107-
}
108-
109113
const hmrEditsWithContent = hmrEdits.map((e) => ({
110114
...e,
111115
content: nodeFs.readFileSync(e.replacementPath, 'utf-8'),
@@ -119,6 +123,9 @@ function main() {
119123
currentArtifactContent = nodeFs.readFileSync(nodeScriptPath);
120124
}
121125

126+
// Snapshot buildSeq before writing so we can detect the resulting build
127+
const preWriteBuildSeq = await getBuildSeq(port);
128+
122129
for (const hmrEdit of hmrEditsWithContent) {
123130
console.log(`🔄 Writing content to: ${hmrEdit.targetPath}`);
124131
nodeFs.writeFileSync(hmrEdit.targetPath, hmrEdit.content);
@@ -127,16 +134,20 @@ function main() {
127134
console.log(`⏳ Waiting for HMR to be triggered for step ${step}`);
128135

129136
if (needRestart || needReload) {
130-
// Waiting Reload hmr update to be triggered. If we close the process too fast, dev engine will think there're no clients.
131-
// No hmr update will be triggered.
132-
await sensibleTimeoutInMs(2000);
133137
if (needReload) {
138+
// For reload steps, send the 'r' signal to request a rebuild, which
139+
// calls ensureLatestBuildOutput only when the output is stale.
140+
// We don't rely on the watcher since the edited file may be new
141+
// and not yet in the build graph.
134142
console.log(`🏃‍➡️ Sent rebuild message to the dev server`);
135143
devServeProcess.stdin.write('r');
144+
} else {
145+
// For restart steps (no reload), wait for the watcher-triggered build.
146+
await waitForNextBuild(port, preWriteBuildSeq);
136147
}
137148
await runningArtifactProcess.close();
138149
await waitForFileToBeModified(nodeScriptPath, currentArtifactContent);
139-
runningArtifactProcess = await runArtifactProcess(nodeScriptPath, tmpProjectPath);
150+
runningArtifactProcess = await runArtifactProcess(nodeScriptPath, tmpProjectPath, port);
140151
}
141152
await waitForPathExists(nodePath.join(tmpProjectPath, `ok-${index}`), 10 * 1000);
142153
console.log(`✅ HMR triggered for step ${step}`);
@@ -161,7 +172,7 @@ function main() {
161172

162173
let id = 0;
163174

164-
async function runArtifactProcess(artifactPath: string, tmpProjectPath: string) {
175+
async function runArtifactProcess(artifactPath: string, tmpProjectPath: string, port: number) {
165176
const thisId = id;
166177
id++;
167178

@@ -173,6 +184,9 @@ async function runArtifactProcess(artifactPath: string, tmpProjectPath: string)
173184
`.trim(),
174185
);
175186

187+
// Snapshot registered clients before starting the process
188+
const currentRegistered = await getModuleRegistrationSeq(port);
189+
176190
console.log(`🔄 Starting Node.js process: ${artifactPath}`);
177191
const artifactProcess = execa(
178192
'node',
@@ -183,7 +197,8 @@ async function runArtifactProcess(artifactPath: string, tmpProjectPath: string)
183197
// Wait for the Node.js process to start
184198
await waitForPathExists(initOkFilePath);
185199

186-
await sensibleTimeoutInMs(2000); // Make sure module are registered
200+
// Wait for modules to be registered with the dev server
201+
await waitForModuleRegistration(port, currentRegistered);
187202

188203
return {
189204
process: artifactProcess,

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { setTimeout } from 'node:timers/promises';
22
import { describe, expect, test } from 'vitest';
3-
import { editFile, getPage } from './test-utils';
3+
import { editFile, getPage, waitForBuildStable } from './test-utils';
44

55
describe('hmr-full-bundle-mode', () => {
66
test.sequential('should render initial content', async () => {
@@ -23,12 +23,16 @@ describe('hmr-full-bundle-mode', () => {
2323

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

26+
// Wait for the build to stabilize before the next edit so the watcher's
27+
// debounce window has closed and will detect the new change.
28+
await waitForBuildStable(3000);
2629
await editFile('hmr.js', (code) =>
2730
code.replace("const foo = 'hello1'", "const foo = 'hello2'"),
2831
);
2932

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

35+
await waitForBuildStable(3000);
3236
await editFile('hmr.js', (code) => code.replace("const foo = 'hello2'", "const foo = 'hello'"));
3337
await expect.poll(() => page.textContent('.hmr')).toBe('hello');
3438
});

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@ import nodeAssert from 'node:assert';
22
import nodeFs from 'node:fs';
33
import nodePath from 'node:path';
44

5-
import { getDevWatchOptionsForCi } from '@rolldown/test-dev-server';
6-
75
// `/packages/test-dev-server/tests`
86
const testsDir = nodePath.resolve(import.meta.dirname, '..').normalize();
97
nodeAssert.ok(nodeFs.existsSync(nodePath.join(testsDir, 'playground')));
@@ -16,5 +14,4 @@ export const CONFIG = {
1614
hmrFullBundleModeDir: nodePath.join(testsDir, 'playground/hmr-full-bundle-mode'),
1715
tmpFullBundleModeDir: nodePath.join(testsDir, 'tmp-playground/hmr-full-bundle-mode'),
1816
},
19-
watch: getDevWatchOptionsForCi(),
2017
};

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

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,6 @@ export function removeDirSync(path: string) {
1616
}
1717
}
1818

19-
export function sensibleTimeoutInMs(ms: number) {
20-
const actualMs = process.env.CI ? ms * 3 : ms;
21-
22-
return new Promise<void>((resolve) => {
23-
setTimeout(resolve, actualMs);
24-
});
25-
}
26-
2719
export async function isDirectoryExists(path: string): Promise<boolean> {
2820
try {
2921
return await nodeFs.promises.access(path).then(() => true);

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

Lines changed: 112 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,23 @@
11
import nodeFs from 'node:fs';
22
import { resolve } from 'node:path';
3+
import { vi } from 'vitest';
4+
import { getDevWatchOptionsForCi } from '@rolldown/test-dev-server';
35
import { CONFIG } from './src/config.js';
46

57
const testDir = CONFIG.paths.tmpFullBundleModeDir;
68

9+
/** Timeout (ms) for individual fetch requests to /_dev/status. */
10+
const FETCH_TIMEOUT_MS = 5_000;
11+
12+
/**
13+
* Minimum time (ms) buildSeq must remain unchanged to consider the build stable.
14+
* Derived from the actual watcher debounce config with margin.
15+
*/
16+
const BUILD_STABLE_MS = (() => {
17+
const opts = getDevWatchOptionsForCi();
18+
return opts.debounceDuration + opts.debounceTickRate + 200;
19+
})();
20+
721
/**
822
* Edit a file using Node.js fs module
923
* Files are edited in the tmp directory, not the original source
@@ -20,10 +34,6 @@ export async function editFile(
2034
return;
2135
}
2236
nodeFs.writeFileSync(filePath, newContent, 'utf-8');
23-
24-
// Small delay to ensure file system events are picked up
25-
await new Promise((resolve) => setTimeout(resolve, 1000));
26-
2737
console.log(`[editFile] Updated ${filename}`);
2838
}
2939

@@ -37,3 +47,101 @@ export function getPage() {
3747
}
3848
return page;
3949
}
50+
51+
interface DevStatus {
52+
hasStaleOutput: boolean;
53+
lastFullBuildFailed: boolean;
54+
buildSeq: number;
55+
connectedClients: number;
56+
moduleRegistrationSeq: number;
57+
}
58+
59+
async function fetchDevStatus(port: number): Promise<DevStatus> {
60+
const res = await fetch(`http://localhost:${port}/_dev/status`, {
61+
signal: AbortSignal.timeout(FETCH_TIMEOUT_MS),
62+
});
63+
if (!res.ok) {
64+
throw new Error(`/_dev/status responded with ${res.status}`);
65+
}
66+
return await res.json();
67+
}
68+
69+
/** Poll until buildSeq increments past the given value (i.e., a new build completed). */
70+
export async function waitForNextBuild(
71+
port: number,
72+
currentBuildSeq: number,
73+
timeoutMs = 30_000,
74+
): Promise<DevStatus> {
75+
return vi.waitFor(
76+
async () => {
77+
const status = await fetchDevStatus(port);
78+
if (status.buildSeq > currentBuildSeq) return status;
79+
throw new Error(`buildSeq still at ${status.buildSeq}, waiting for > ${currentBuildSeq}`);
80+
},
81+
{ timeout: timeoutMs, interval: 50 },
82+
);
83+
}
84+
85+
/**
86+
* Wait for buildSeq to stabilize (no changes for `stableMs`). This ensures the debounce window has closed.
87+
*
88+
* Uses BUILD_STABLE_MS derived from the actual watcher debounce config.
89+
*/
90+
export async function waitForBuildStable(
91+
port: number,
92+
stableMs = BUILD_STABLE_MS,
93+
timeoutMs = 30_000,
94+
): Promise<DevStatus> {
95+
const start = Date.now();
96+
let lastSeq = -1;
97+
let lastChangeTime = start;
98+
let lastError: unknown;
99+
while (Date.now() - start < timeoutMs) {
100+
try {
101+
const status = await fetchDevStatus(port);
102+
if (status.buildSeq !== lastSeq) {
103+
lastSeq = status.buildSeq;
104+
lastChangeTime = Date.now();
105+
} else if (Date.now() - lastChangeTime >= stableMs) {
106+
return status;
107+
}
108+
} catch (e) {
109+
lastError = e;
110+
}
111+
await new Promise((r) => setTimeout(r, 50));
112+
}
113+
throw new Error(
114+
`Build not stable within ${timeoutMs}ms` +
115+
(lastError ? `. Last fetch error: ${lastError}` : ''),
116+
);
117+
}
118+
119+
/** Poll until moduleRegistrationSeq exceeds the given value (i.e., a new module registration happened). */
120+
export async function waitForModuleRegistration(
121+
port: number,
122+
currentSeq: number,
123+
timeoutMs = 30_000,
124+
): Promise<DevStatus> {
125+
return vi.waitFor(
126+
async () => {
127+
const status = await fetchDevStatus(port);
128+
if (status.moduleRegistrationSeq > currentSeq) return status;
129+
throw new Error(
130+
`moduleRegistrationSeq still at ${status.moduleRegistrationSeq}, waiting for > ${currentSeq}`,
131+
);
132+
},
133+
{ timeout: timeoutMs, interval: 50 },
134+
);
135+
}
136+
137+
/** Get current module registration sequence number. */
138+
export async function getModuleRegistrationSeq(port: number): Promise<number> {
139+
const status = await fetchDevStatus(port);
140+
return status.moduleRegistrationSeq;
141+
}
142+
143+
/** Get current build sequence number. */
144+
export async function getBuildSeq(port: number): Promise<number> {
145+
const status = await fetchDevStatus(port);
146+
return status.buildSeq;
147+
}

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -141,13 +141,16 @@ beforeAll(async () => {
141141
beforeEach(async (ctx) => {
142142
const retryCount = ctx.task.result?.retryCount ?? 0;
143143
if (retryCount > 0) {
144+
// On retry, restart the dev server with fresh files to avoid stale state
145+
if (devServerProcess) {
146+
devServerProcess.kill('SIGTERM');
147+
devServerProcess = null;
148+
}
149+
await killPort(3000);
144150
await resetTestFiles();
145-
// Wait for file system watcher to detect and process the changes
146-
await new Promise((resolve) => setTimeout(resolve, 1000 * 3));
147-
// Reload the page to ensure it reflects the reset file state
148-
// This is necessary because after a failed test, the page may show stale content
151+
({ devServerProcess } = await startDevServer());
149152
if (page) {
150-
await page.reload({ waitUntil: 'networkidle' });
153+
await page.goto('http://localhost:3000', { waitUntil: 'networkidle' });
151154
}
152155
}
153156
});

0 commit comments

Comments
 (0)