Skip to content

Commit 4023920

Browse files
killaguclaude
andauthored
test: fix flaky CI tests (multipart tmpdir teardown + egg-scripts start readiness) (#6015)
Two unrelated **flaky test** fixes surfaced while stabilizing CI for the snapshot work (#6011). Test-only; no `src` changes. ## 1. `@eggjs/multipart` — `ENOTEMPTY` teardown race `file-mode.test.ts` flaked on macOS Node 24 (passing on every other matrix job): ``` Error: ENOTEMPTY: directory not empty, rmdir '.../egg-multipart-tmp/multipart-file-mode-demo/2026/06/28/05' ``` `fs.rm(tmpdir, { force: true, recursive: true })` defaults to `maxRetries: 0`, so a transient `ENOTEMPTY` (a file in a date-bucket dir mid recursive-removal — async per-request cleanup draining / APFS latency) throws and fails the suite. The production `clean_tmpdir` schedule already swallows these; only the 7 test teardowns are fragile. **Fix:** add `maxRetries: 3` to all 7. ## 2. `@eggjs/scripts` — flaky `stop.test.ts` start readiness `Test scripts (ubuntu-22)` failed in `stop.test.ts`: ``` AssertionError: expected 'Starting custom-framework application…' to match /custom-framework started on http:\/\/…/ ``` `stop.test.ts` waited a fixed **1s** for a 2-worker egg app to boot before asserting its `started on http://…` log — while every sibling `start-without-demon-*.test.ts` waits **10s**. On a loaded runner 1s isn't enough. **Fix:** replace the fixed `scheduler.wait(waitTime)` before each startup/shutdown assertion with a `waitFor(getText, pattern, timeout)` poll (`test/utils.ts`) that returns as soon as the expected output appears (up to 10s) — race-free and faster than a fixed delay. Windows keeps the fixed settle for the signal-driven shutdown checks (SIGTERM isn't handled there). ## Verification - multipart: the 7 teardowns now retry; affected tests pass locally. - scripts: `stop.test.ts` passes locally after build (8 passed, 1 skipped). Re Gemini's review on the multipart hunks: it claimed Vitest runs `afterAll` in registration order — that's incorrect (default `sequence.hooks: 'stack'` → reverse; verified empirically on v4.1.9), so the current teardown already removes the tmpdir **last** (after `server.close()`/`app.close()`); the suggested reorder would move `fs.rm` first and reintroduce the race. Replied inline on each thread. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved cleanup reliability across multipart-related test suites by adding additional retry behavior when removing temporary directories during teardown. * Updated process lifecycle checks in test runs to wait for specific startup/shutdown output instead of relying on fixed timing, reducing flaky boot/shutdown timing. * Introduced a reusable polling helper to detect expected log output within a timeout. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 9e8abce commit 4023920

9 files changed

Lines changed: 45 additions & 13 deletions

plugins/multipart/test/dynamic-option.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ describe('test/dynamic-option.test.ts', () => {
2121
});
2222

2323
afterAll(async () => {
24-
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true });
24+
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true, maxRetries: 3 });
2525
});
2626
afterAll(() => app.close());
2727
afterAll(() => server.close());

plugins/multipart/test/enable-pathToRegexpModule.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ describe.skip('test/enable-pathToRegexpModule.test.ts', () => {
2323
host = 'http://127.0.0.1:' + server.address().port;
2424
});
2525
afterAll(async () => {
26-
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true });
26+
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true, maxRetries: 3 });
2727
});
2828
afterAll(() => app.close());
2929
afterAll(() => server.close());

plugins/multipart/test/file-mode-limit-filesize-per-request.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ describe('test/file-mode-limit-filesize-per-request.test.ts', () => {
2222
host = 'http://127.0.0.1:' + server.address().port;
2323
});
2424
afterAll(async () => {
25-
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true });
25+
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true, maxRetries: 3 });
2626
});
2727
afterAll(() => app.close());
2828
afterAll(() => server.close());

plugins/multipart/test/file-mode.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ describe('test/file-mode.test.ts', () => {
2828
host = 'http://127.0.0.1:' + server.address().port;
2929
});
3030
afterAll(async () => {
31-
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true });
31+
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true, maxRetries: 3 });
3232
});
3333
afterAll(() => app.close());
3434
afterAll(() => server.close());

plugins/multipart/test/stream-mode-with-filematch-glob.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ describe('test/stream-mode-with-filematch-glob.test.ts', () => {
2626
});
2727
afterAll(async () => {
2828
try {
29-
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true });
29+
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true, maxRetries: 3 });
3030
} catch (err) {
3131
console.error(err);
3232
}

plugins/multipart/test/stream-mode-with-filematch.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ describe('test/stream-mode-with-filematch.test.ts', () => {
2626
});
2727
afterAll(async () => {
2828
try {
29-
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true });
29+
await fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true, maxRetries: 3 });
3030
} catch (err) {
3131
console.error(err);
3232
}

plugins/multipart/test/ts.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ describe('test/ts.test.ts', () => {
2525
host = 'http://127.0.0.1:' + server.address().port;
2626
});
2727
afterAll(() => {
28-
return fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true });
28+
return fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true, maxRetries: 3 });
2929
});
3030
afterAll(() => app.close());
3131
afterAll(() => server.close());

tools/scripts/test/stop.test.ts

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { mm, restore } from 'mm';
88
import { describe, it, beforeAll, afterAll, beforeEach, afterEach, expect } from 'vitest';
99

1010
import { isWindows } from '../src/helper.ts';
11-
import { cleanup, replaceWeakRefMessage, type Coffee } from './utils.ts';
11+
import { cleanup, replaceWeakRefMessage, waitFor, type Coffee } from './utils.ts';
1212

1313
const __dirname = import.meta.dirname;
1414

@@ -45,7 +45,9 @@ describe('test/stop.test.ts', () => {
4545
]) as Coffee;
4646
app.debug();
4747
app.expect('code', 0);
48-
await scheduler.wait(waitTime);
48+
// Poll until the app reports it has started instead of a fixed delay: a 2-worker
49+
// egg app can take well over a second to boot on a loaded CI runner.
50+
await waitFor(() => app.stdout, /custom-framework started on http:\/\/127\.0\.0\.1:\d+/);
4951

5052
expect(replaceWeakRefMessage(app.stderr)).toBe('');
5153
expect(app.stdout).toMatch(/custom-framework started on http:\/\/127\.0\.0\.1:\d+/);
@@ -155,7 +157,8 @@ describe('test/stop.test.ts', () => {
155157
]) as Coffee;
156158
// app.debug();
157159
app.expect('code', 0);
158-
await scheduler.wait(waitTime);
160+
// Poll until the app reports it has started (see note above).
161+
await waitFor(() => app.stdout, /custom-framework started on http:\/\/127\.0\.0\.1:\d+/);
159162

160163
expect(replaceWeakRefMessage(app.stderr)).toBe('');
161164
expect(app.stdout).toMatch(/custom-framework started on http:\/\/127\.0\.0\.1:\d+/);
@@ -314,7 +317,8 @@ describe('test/stop.test.ts', () => {
314317
app.debug();
315318
app.expect('code', 0);
316319

317-
await scheduler.wait(waitTime);
320+
// Poll until the app reports it has started (see note above).
321+
await waitFor(() => app.stdout, /http:\/\/127\.0\.0\.1:\d+/);
318322

319323
// assert.equal(replaceWeakRefMessage(app.stderr), '');
320324
expect(app.stdout).toMatch(/http:\/\/127\.0\.0\.1:\d+/);
@@ -332,7 +336,13 @@ describe('test/stop.test.ts', () => {
332336
killer.debug();
333337
killer.expect('code', 0);
334338
await killer.end();
335-
await scheduler.wait(waitTime);
339+
// Poll until the app logs its shutdown instead of a fixed delay. On Windows the
340+
// SIGTERM signal is not handled (no shutdown log), so keep a fixed settle there.
341+
if (isWindows) {
342+
await scheduler.wait(waitTime);
343+
} else {
344+
await waitFor(() => app.stdout, /\[master] master is killed by signal SIGTERM, closing/);
345+
}
336346

337347
// make sure is kill not auto exist
338348
expect(app.stdout).not.toMatch(/exist by env/);
@@ -356,7 +366,13 @@ describe('test/stop.test.ts', () => {
356366
killer.expect('code', 0);
357367

358368
// await killer.end();
359-
await scheduler.wait(waitTime);
369+
// Poll until the app has fully exited (master exits last, after workers/agent),
370+
// instead of a fixed delay. On Windows the signal is not handled, so settle fixed.
371+
if (isWindows) {
372+
await scheduler.wait(waitTime);
373+
} else {
374+
await waitFor(() => app.stdout, /\[master] exit with code:0/);
375+
}
360376

361377
// make sure is kill not auto exist
362378
expect(app.stdout).not.toMatch(/exist by env/);

tools/scripts/test/utils.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,3 +55,19 @@ export function replaceWeakRefMessage(stderr: string) {
5555
}
5656
return stderr;
5757
}
58+
59+
/**
60+
* Poll until `getText()` matches `pattern`, up to `timeout` ms (default 10s),
61+
* checking every 100ms. Returns as soon as it matches, and resolves anyway on
62+
* timeout so the caller's own `expect(...).toMatch(...)` still produces a useful
63+
* diff. Use this instead of a fixed `scheduler.wait(n)` before asserting on a
64+
* forked process's stdout: a loaded CI runner may not have finished booting (or
65+
* shutting down) within `n`, which is the classic source of these flaky timeouts.
66+
*/
67+
export async function waitFor(getText: () => string, pattern: RegExp, timeout = 10000): Promise<void> {
68+
const deadline = Date.now() + timeout;
69+
while (!pattern.test(getText())) {
70+
if (Date.now() >= deadline) return;
71+
await scheduler.wait(100);
72+
}
73+
}

0 commit comments

Comments
 (0)