Skip to content

Commit 8bb708b

Browse files
authored
test: unflake inspector-cli tests (microsoft#22347)
This patch: - changes the `childProcess` fixture to reliably SIGKILL all descendants (children and grand-children, regardless of their process group). This is achieved using the `ps` command to build the process tree, and then send `SIGKILL` to the descendant process groups. - changes the `runCLI` fixture to **not** auto-close codegen by default; the `childProcess` fixture will clean up all processes. This makes sure that all `runCLI.waitFor()` commands actually wait until the necessary output. - for a handful of tests that do actually want to auto-close codegen, introduce an optional `autoCloseWhen` flag for the `runCLI` fixture that makes sure to close the codegen once a certain output was reached.
1 parent 56dcab8 commit 8bb708b

16 files changed

+177
-154
lines changed

packages/playwright-core/src/cli/cli.ts

+18-7
Original file line numberDiff line numberDiff line change
@@ -414,7 +414,16 @@ async function launchContext(options: Options, headless: boolean, executablePath
414414

415415
const browser = await browserType.launch(launchOptions);
416416

417-
if (process.env.PWTEST_CLI_EXIT) {
417+
if (process.env.PWTEST_CLI_IS_UNDER_TEST) {
418+
(process as any)._didSetSourcesForTest = (text: string) => {
419+
process.stdout.write('\n-------------8<-------------\n');
420+
process.stdout.write(text);
421+
process.stdout.write('\n-------------8<-------------\n');
422+
const autoExitCondition = process.env.PWTEST_CLI_AUTO_EXIT_WHEN;
423+
if (autoExitCondition && text.includes(autoExitCondition))
424+
Promise.all(context.pages().map(async p => p.close()));
425+
};
426+
// Make sure we exit abnormally when browser crashes.
418427
const logs: string[] = [];
419428
require('playwright-core/lib/utilsBundle').debug.log = (...args: any[]) => {
420429
const line = require('util').format(...args) + '\n';
@@ -425,7 +434,6 @@ async function launchContext(options: Options, headless: boolean, executablePath
425434
const hasCrashLine = logs.some(line => line.includes('process did exit:') && !line.includes('process did exit: exitCode=0, signal=null'));
426435
if (hasCrashLine) {
427436
process.stderr.write('Detected browser crash.\n');
428-
// Make sure we exit abnormally when browser crashes.
429437
process.exit(1);
430438
}
431439
});
@@ -552,7 +560,14 @@ async function openPage(context: BrowserContext, url: string | undefined): Promi
552560
url = 'file://' + path.resolve(url);
553561
else if (!url.startsWith('http') && !url.startsWith('file://') && !url.startsWith('about:') && !url.startsWith('data:'))
554562
url = 'http://' + url;
555-
await page.goto(url);
563+
await page.goto(url).catch(error => {
564+
if (process.env.PWTEST_CLI_AUTO_EXIT_WHEN && error.message.includes('Navigation failed because page was closed')) {
565+
// Tests with PWTEST_CLI_AUTO_EXIT_WHEN might close page too fast, resulting
566+
// in a stray navigation aborted error. We should ignore it.
567+
} else {
568+
throw error;
569+
}
570+
});
556571
}
557572
return page;
558573
}
@@ -567,8 +582,6 @@ async function open(options: Options, url: string | undefined, language: string)
567582
saveStorage: options.saveStorage,
568583
});
569584
await openPage(context, url);
570-
if (process.env.PWTEST_CLI_EXIT)
571-
await Promise.all(context.pages().map(p => p.close()));
572585
}
573586

574587
async function codegen(options: Options, url: string | undefined, language: string, outputFile?: string) {
@@ -584,8 +597,6 @@ async function codegen(options: Options, url: string | undefined, language: stri
584597
handleSIGINT: false,
585598
});
586599
await openPage(context, url);
587-
if (process.env.PWTEST_CLI_EXIT)
588-
await Promise.all(context.pages().map(p => p.close()));
589600
}
590601

591602
async function waitForPage(page: Page, captureOptions: CaptureOptions) {

packages/playwright-core/src/server/recorder/recorderApp.ts

+2-7
Original file line numberDiff line numberDiff line change
@@ -170,13 +170,8 @@ export class RecorderApp extends EventEmitter implements IRecorderApp {
170170
}).toString(), true, sources, 'main').catch(() => {});
171171

172172
// Testing harness for runCLI mode.
173-
{
174-
if ((process.env.PWTEST_CLI_IS_UNDER_TEST || process.env.PWTEST_CLI_EXIT) && sources.length) {
175-
process.stdout.write('\n-------------8<-------------\n');
176-
process.stdout.write(sources[0].text);
177-
process.stdout.write('\n-------------8<-------------\n');
178-
}
179-
}
173+
if (process.env.PWTEST_CLI_IS_UNDER_TEST && sources.length)
174+
(process as any)._didSetSourcesForTest(sources[0].text);
180175
}
181176

182177
async setSelector(selector: string, focus?: boolean): Promise<void> {

tests/config/commonFixtures.ts

+72-28
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,38 @@ type TestChildParams = {
2828
onOutput?: () => void;
2929
};
3030

31+
import childProcess from 'child_process';
32+
33+
type ProcessData = {
34+
pid: number, // process ID
35+
pgid: number, // process groupd ID
36+
children: Set<ProcessData>, // direct children of the process
37+
};
38+
39+
function buildProcessTreePosix(pid: number): ProcessData {
40+
const processTree = childProcess.spawnSync('ps', ['-eo', 'pid,pgid,ppid']);
41+
const lines = processTree.stdout.toString().trim().split('\n');
42+
43+
const pidToProcess = new Map<number, ProcessData>();
44+
const edges: { pid: number, ppid: number }[] = [];
45+
for (const line of lines) {
46+
const [pid, pgid, ppid] = line.trim().split(/\s+/).map(token => +token);
47+
// On linux, the very first line of `ps` is the header with "PID PGID PPID".
48+
if (isNaN(pid) || isNaN(pgid) || isNaN(ppid))
49+
continue;
50+
pidToProcess.set(pid, { pid, pgid, children: new Set() });
51+
edges.push({ pid, ppid });
52+
}
53+
for (const { pid, ppid } of edges) {
54+
const parent = pidToProcess.get(ppid);
55+
const child = pidToProcess.get(pid);
56+
// On POSIX, certain processes might not have parent (e.g. PID=1 and occasionally PID=2).
57+
if (parent && child)
58+
parent.children.add(child);
59+
}
60+
return pidToProcess.get(pid);
61+
}
62+
3163
export class TestChildProcess {
3264
params: TestChildParams;
3365
process: ChildProcess;
@@ -72,7 +104,7 @@ export class TestChildProcess {
72104
this.process.stderr.on('data', appendChunk);
73105
this.process.stdout.on('data', appendChunk);
74106

75-
const killProcessGroup = this._killProcessGroup.bind(this);
107+
const killProcessGroup = this._killProcessTree.bind(this, 'SIGKILL');
76108
process.on('exit', killProcessGroup);
77109
this.exited = new Promise(f => {
78110
this.process.on('exit', (exitCode, signal) => f({ exitCode, signal }));
@@ -86,28 +118,49 @@ export class TestChildProcess {
86118
return strippedOutput.split('\n').filter(line => line.startsWith('%%')).map(line => line.substring(2).trim());
87119
}
88120

89-
async close() {
90-
if (this.process.kill(0))
91-
this._killProcessGroup('SIGINT');
121+
async kill(signal: 'SIGINT' | 'SIGKILL' = 'SIGKILL') {
122+
this._killProcessTree(signal);
92123
return this.exited;
93124
}
94125

95-
async kill() {
96-
if (this.process.kill(0))
97-
this._killProcessGroup('SIGKILL');
98-
return this.exited;
99-
}
100-
101-
private _killProcessGroup(signal: 'SIGINT' | 'SIGKILL') {
126+
private _killProcessTree(signal: 'SIGINT' | 'SIGKILL') {
102127
if (!this.process.pid || !this.process.kill(0))
103128
return;
104-
try {
105-
if (process.platform === 'win32')
129+
130+
// On Windows, we always call `taskkill` no matter signal.
131+
if (process.platform === 'win32') {
132+
try {
106133
execSync(`taskkill /pid ${this.process.pid} /T /F /FI "MEMUSAGE gt 0"`, { stdio: 'ignore' });
107-
else
108-
process.kill(-this.process.pid, signal);
109-
} catch (e) {
110-
// the process might have already stopped
134+
} catch (e) {
135+
// the process might have already stopped
136+
}
137+
return;
138+
}
139+
140+
// In case of POSIX and `SIGINT` signal, send it to the main process group only.
141+
if (signal === 'SIGINT') {
142+
try {
143+
process.kill(-this.process.pid, 'SIGINT');
144+
} catch (e) {
145+
// the process might have already stopped
146+
}
147+
return;
148+
}
149+
150+
// In case of POSIX and `SIGKILL` signal, we should send it to all descendant process groups.
151+
const rootProcess = buildProcessTreePosix(this.process.pid);
152+
const descendantProcessGroups = (function flatten(processData: ProcessData, result: Set<number> = new Set()) {
153+
// Process can nullify its own process group with `setpgid`. Use its PID instead.
154+
result.add(processData.pgid || processData.pid);
155+
processData.children.forEach(child => flatten(child, result));
156+
return result;
157+
})(rootProcess);
158+
for (const pgid of descendantProcessGroups) {
159+
try {
160+
process.kill(-pgid, 'SIGKILL');
161+
} catch (e) {
162+
// the process might have already stopped
163+
}
111164
}
112165
}
113166

@@ -150,16 +203,7 @@ export const commonFixtures: Fixtures<CommonFixtures, CommonWorkerFixtures> = {
150203
processes.push(process);
151204
return process;
152205
});
153-
await Promise.all(processes.map(async child => {
154-
await Promise.race([
155-
child.exited,
156-
new Promise(f => setTimeout(f, 3_000)),
157-
]);
158-
if (child.process.kill(0)) {
159-
await child.kill();
160-
throw new Error(`Process ${child.params.command.join(' ')} is still running. Leaking process?\nOutput:${child.output}`);
161-
}
162-
}));
206+
await Promise.all(processes.map(async child => child.kill()));
163207
if (testInfo.status !== 'passed' && testInfo.status !== 'skipped' && !process.env.PWTEST_DEBUG) {
164208
for (const process of processes) {
165209
console.log('====== ' + process.params.command.join(' '));
@@ -176,7 +220,7 @@ export const commonFixtures: Fixtures<CommonFixtures, CommonWorkerFixtures> = {
176220
processes.push(process);
177221
return process;
178222
});
179-
await Promise.all(processes.map(child => child.close()));
223+
await Promise.all(processes.map(child => child.kill('SIGINT')));
180224
}, { scope: 'worker' }],
181225

182226
waitForPort: async ({}, use) => {

tests/config/remoteServer.ts

+2-3
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,7 @@ export class RunServer implements PlaywrightServer {
4949
}
5050

5151
async close() {
52-
await this._process.close();
53-
await this._process.exitCode;
52+
await this._process.kill('SIGINT');
5453
}
5554
}
5655

@@ -150,7 +149,7 @@ export class RemoteServer implements PlaywrightServer {
150149
await this._browser.close();
151150
this._browser = undefined;
152151
}
153-
await this._process.close();
152+
await this._process.kill('SIGINT');
154153
await this.childExitCode();
155154
}
156155
}

tests/installation/playwright-cli-codegen.spec.ts

+18-6
Original file line numberDiff line numberDiff line change
@@ -19,20 +19,32 @@ test('codegen should work', async ({ exec }) => {
1919
await exec('npm i --foreground-scripts playwright');
2020

2121
await test.step('codegen without arguments', async () => {
22-
const result = await exec('npx playwright codegen', { env: { PWTEST_CLI_EXIT: '1' } });
23-
expect(result).toContain(`@playwright/test`);
22+
const result = await exec('npx playwright codegen', {
23+
env: {
24+
PWTEST_CLI_IS_UNDER_TEST: '1',
25+
PWTEST_CLI_AUTO_EXIT_WHEN: '@playwright/test',
26+
}
27+
});
2428
expect(result).toContain(`{ page }`);
2529
});
2630

2731
await test.step('codegen --target=javascript', async () => {
28-
const result = await exec('npx playwright codegen --target=javascript', { env: { PWTEST_CLI_EXIT: '1' } });
32+
const result = await exec('npx playwright codegen --target=javascript', {
33+
env: {
34+
PWTEST_CLI_IS_UNDER_TEST: '1',
35+
PWTEST_CLI_AUTO_EXIT_WHEN: 'context.close',
36+
}
37+
});
2938
expect(result).toContain(`playwright`);
30-
expect(result).toContain(`page.close`);
3139
});
3240

3341
await test.step('codegen --target=python', async () => {
34-
const result = await exec('npx playwright codegen --target=python', { env: { PWTEST_CLI_EXIT: '1' } });
35-
expect(result).toContain(`chromium.launch`);
42+
const result = await exec('npx playwright codegen --target=python', {
43+
env: {
44+
PWTEST_CLI_IS_UNDER_TEST: '1',
45+
PWTEST_CLI_AUTO_EXIT_WHEN: 'chromium.launch',
46+
},
47+
});
3648
expect(result).toContain(`browser.close`);
3749
});
3850
});

tests/library/inspector/cli-codegen-2.spec.ts

+6-6
Original file line numberDiff line numberDiff line change
@@ -481,8 +481,10 @@ test.describe('cli codegen', () => {
481481

482482
test('should --save-trace', async ({ runCLI }, testInfo) => {
483483
const traceFileName = testInfo.outputPath('trace.zip');
484-
const cli = runCLI([`--save-trace=${traceFileName}`]);
485-
await cli.exited;
484+
const cli = runCLI([`--save-trace=${traceFileName}`], {
485+
autoExitWhen: ' ',
486+
});
487+
await cli.waitForCleanExit();
486488
expect(fs.existsSync(traceFileName)).toBeTruthy();
487489
});
488490

@@ -492,11 +494,9 @@ test.describe('cli codegen', () => {
492494
const traceFileName = testInfo.outputPath('trace.zip');
493495
const storageFileName = testInfo.outputPath('auth.json');
494496
const harFileName = testInfo.outputPath('har.har');
495-
const cli = runCLI([`--save-trace=${traceFileName}`, `--save-storage=${storageFileName}`, `--save-har=${harFileName}`], {
496-
noAutoExit: true,
497-
});
497+
const cli = runCLI([`--save-trace=${traceFileName}`, `--save-storage=${storageFileName}`, `--save-har=${harFileName}`]);
498498
await cli.waitFor(`import { test, expect } from '@playwright/test'`);
499-
cli.exit('SIGINT');
499+
cli.process.kill('SIGINT');
500500
const { exitCode } = await cli.process.exited;
501501
expect(exitCode).toBe(130);
502502
expect(fs.existsSync(traceFileName)).toBeTruthy();

tests/library/inspector/cli-codegen-csharp.spec.ts

+5-10
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,7 @@ class Program
4343
${launchOptions(channel)}
4444
});
4545
var context = await browser.NewContextAsync();`;
46-
await cli.waitFor(expectedResult).catch(e => e);
47-
expect(cli.text()).toContain(expectedResult);
46+
await cli.waitFor(expectedResult);
4847
});
4948

5049
test('should print the correct context options for custom settings', async ({ browserName, channel, runCLI }) => {
@@ -87,7 +86,6 @@ test('should print the correct context options for custom settings', async ({ br
8786
},
8887
});`;
8988
await cli.waitFor(expectedResult);
90-
expect(cli.text()).toContain(expectedResult);
9189
});
9290

9391
test('should print the correct context options when using a device', async ({ browserName, channel, runCLI }) => {
@@ -102,7 +100,6 @@ test('should print the correct context options when using a device', async ({ br
102100
});
103101
var context = await browser.NewContextAsync(playwright.Devices["Pixel 2"]);`;
104102
await cli.waitFor(expectedResult);
105-
expect(cli.text()).toContain(expectedResult);
106103
});
107104

108105
test('should print the correct context options when using a device and additional options', async ({ browserName, channel, runCLI }) => {
@@ -147,9 +144,7 @@ test('should print the correct context options when using a device and additiona
147144
Width = 1280,
148145
},
149146
});`;
150-
151147
await cli.waitFor(expectedResult);
152-
expect(cli.text()).toContain(expectedResult);
153148
});
154149

155150
test('should print load/save storageState', async ({ browserName, channel, runCLI }, testInfo) => {
@@ -179,17 +174,17 @@ test('should print load/save storageState', async ({ browserName, channel, runCL
179174

180175
test('should work with --save-har', async ({ runCLI }, testInfo) => {
181176
const harFileName = testInfo.outputPath('har.har');
182-
const cli = runCLI(['--target=csharp', `--save-har=${harFileName}`]);
183177
const expectedResult = `
184178
var context = await browser.NewContextAsync(new BrowserNewContextOptions
185179
{
186180
RecordHarMode = HarMode.Minimal,
187181
RecordHarPath = ${JSON.stringify(harFileName)},
188182
ServiceWorkers = ServiceWorkerPolicy.Block,
189183
});`;
190-
await cli.waitFor(expectedResult).catch(e => e);
191-
expect(cli.text()).toContain(expectedResult);
192-
await cli.exited;
184+
const cli = runCLI(['--target=csharp', `--save-har=${harFileName}`], {
185+
autoExitWhen: expectedResult,
186+
});
187+
await cli.waitForCleanExit();
193188
const json = JSON.parse(fs.readFileSync(harFileName, 'utf-8'));
194189
expect(json.log.creator.name).toBe('Playwright');
195190
});

tests/library/inspector/cli-codegen-java.spec.ts

+5-6
Original file line numberDiff line numberDiff line change
@@ -37,15 +37,13 @@ public class Example {
3737
${launchOptions(channel)});
3838
BrowserContext context = browser.newContext();`;
3939
await cli.waitFor(expectedResult);
40-
expect(cli.text()).toContain(expectedResult);
4140
});
4241

4342
test('should print the correct context options for custom settings', async ({ runCLI, browserName }) => {
4443
const cli = runCLI(['--color-scheme=light', '--target=java', emptyHTML]);
4544
const expectedResult = `BrowserContext context = browser.newContext(new Browser.NewContextOptions()
4645
.setColorScheme(ColorScheme.LIGHT));`;
4746
await cli.waitFor(expectedResult);
48-
expect(cli.text()).toContain(expectedResult);
4947
});
5048

5149
test('should print the correct context options when using a device', async ({ browserName, runCLI }) => {
@@ -93,14 +91,15 @@ test('should print load/save storage_state', async ({ runCLI, browserName }, tes
9391

9492
test('should work with --save-har', async ({ runCLI }, testInfo) => {
9593
const harFileName = testInfo.outputPath('har.har');
96-
const cli = runCLI(['--target=java', `--save-har=${harFileName}`]);
9794
const expectedResult = `BrowserContext context = browser.newContext(new Browser.NewContextOptions()
9895
.setRecordHarMode(HarMode.MINIMAL)
9996
.setRecordHarPath(Paths.get(${JSON.stringify(harFileName)}))
10097
.setServiceWorkers(ServiceWorkerPolicy.BLOCK));`;
101-
await cli.waitFor(expectedResult).catch(e => e);
102-
expect(cli.text()).toContain(expectedResult);
103-
await cli.exited;
98+
const cli = runCLI(['--target=java', `--save-har=${harFileName}`], {
99+
autoExitWhen: expectedResult,
100+
});
101+
102+
await cli.waitForCleanExit();
104103
const json = JSON.parse(fs.readFileSync(harFileName, 'utf-8'));
105104
expect(json.log.creator.name).toBe('Playwright');
106105
});

0 commit comments

Comments
 (0)