From a156cfe451684cbcf48624a501263808feb5ce88 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Thu, 7 Nov 2024 09:54:32 +0100 Subject: [PATCH] implement proposal --- docs/src/test-api/class-testconfig.md | 2 +- docs/src/test-webserver-js.md | 2 +- .../playwright/src/plugins/webServerPlugin.ts | 42 +++++++++++++------ packages/playwright/types/test.d.ts | 8 ++-- tests/playwright-test/web-server.spec.ts | 35 ++++++++++------ 5 files changed, 60 insertions(+), 29 deletions(-) diff --git a/docs/src/test-api/class-testconfig.md b/docs/src/test-api/class-testconfig.md index af631ca98e..c428d6f405 100644 --- a/docs/src/test-api/class-testconfig.md +++ b/docs/src/test-api/class-testconfig.md @@ -619,7 +619,7 @@ export default defineConfig({ - `stdout` ?<["pipe"|"ignore"]> If `"pipe"`, it will pipe the stdout of the command to the process stdout. If `"ignore"`, it will ignore the stdout of the command. Default to `"ignore"`. - `stderr` ?<["pipe"|"ignore"]> Whether to pipe the stderr of the command to the process stderr or ignore it. Defaults to `"pipe"`. - `timeout` ?<[int]> How long to wait for the process to start up and be available in milliseconds. Defaults to 60000. - - `shutdownTimeout` ?<[int]> How long to wait for the process to gracefully shut down after it was sent `SIGINT`. If exceeded, process is killed with `SIGKILL`. Defaults to 500 milliseconds. Set to 0 to send `SIGKILL` immediately. + - `kill` ?<{ SIGINT: number } | { SIGTERM: number }> How to shut down the process gracefully. If unspecified, the process group is forcefully `SIGKILL`ed. If set to `{ SIGINT: 500 }`, the top process is sent a `SIGINT` signal, followed by `SIGKILL` if it doesn't exit within 500ms. You can also use `SIGTERM` instead. A `0` timeout means no `SIGKILL` will be sent. Windows doesn't support `SIGINT` and `SIGTERM` signals, so this option is ignored. - `url` ?<[string]> The url on your http server that is expected to return a 2xx, 3xx, 400, 401, 402, or 403 status code when the server is ready to accept connections. Redirects (3xx status codes) are being followed and the new location is checked. Either `port` or `url` should be specified. Launch a development web server (or multiple) during the tests. diff --git a/docs/src/test-webserver-js.md b/docs/src/test-webserver-js.md index 9cc0c90dfd..8739207e7f 100644 --- a/docs/src/test-webserver-js.md +++ b/docs/src/test-webserver-js.md @@ -37,7 +37,7 @@ export default defineConfig({ | `stdout` | If `"pipe"`, it will pipe the stdout of the command to the process stdout. If `"ignore"`, it will ignore the stdout of the command. Default to `"ignore"`. | | `stderr` | Whether to pipe the stderr of the command to the process stderr or ignore it. Defaults to `"pipe"`. | | `timeout` | How long to wait for the process to start up and be available in milliseconds. Defaults to 60000. | -| `shutdownTimeout` | How long to wait for the process to gracefully shut down after it was sent `SIGINT`. If exceeded, process is killed with `SIGKILL`. Defaults to 500 milliseconds. Set to 0 to send `SIGKILL` immediately. | +| `kill` | How to shut down the process gracefully. If unspecified, the process group is forcefully `SIGKILL`ed. If set to `{ SIGINT: 500 }`, the top process is sent a `SIGINT` signal, followed by `SIGKILL` if it doesn't exit within 500ms. You can also use `SIGTERM` instead. A `0` timeout means no `SIGKILL` will be sent. Windows doesn't support `SIGINT` and `SIGTERM` signals, so this option is ignored. | ## Adding a server timeout diff --git a/packages/playwright/src/plugins/webServerPlugin.ts b/packages/playwright/src/plugins/webServerPlugin.ts index b1ae5d1cc6..5eda8ee822 100644 --- a/packages/playwright/src/plugins/webServerPlugin.ts +++ b/packages/playwright/src/plugins/webServerPlugin.ts @@ -31,7 +31,7 @@ export type WebServerPluginOptions = { url?: string; ignoreHTTPSErrors?: boolean; timeout?: number; - shutdownTimeout?: number; + kill?: { SIGINT: number }|{ SIGTERM: number }; reuseExistingServer?: boolean; cwd?: string; env?: { [key: string]: string; }; @@ -93,6 +93,19 @@ export class WebServerPlugin implements TestRunnerPlugin { throw new Error(`${this._options.url ?? `http://localhost${port ? ':' + port : ''}`} is already used, make sure that nothing is running on the port/url or set reuseExistingServer:true in config.webServer.`); } + let signal: 'SIGINT' | 'SIGTERM' | undefined = undefined; + let timeout = 0; + if (this._options.kill) { + if ('SIGINT' in this._options.kill && typeof this._options.kill.SIGINT === 'number') { + signal = 'SIGINT'; + timeout = this._options.kill.SIGINT; + } + if ('SIGTERM' in this._options.kill && typeof this._options.kill.SIGTERM === 'number') { + signal = 'SIGTERM'; + timeout = this._options.kill.SIGTERM; + } + } + debugWebServer(`Starting WebServer process ${this._options.command}...`); const { launchedProcess, gracefullyClose } = await launchProcess({ command: this._options.command, @@ -107,21 +120,26 @@ export class WebServerPlugin implements TestRunnerPlugin { attemptToGracefullyClose: async () => { if (process.platform === 'win32') throw new Error('Graceful shutdown is not supported on Windows'); - - if (this._options.shutdownTimeout === 0) + if (!signal) throw new Error('skip graceful shutdown'); - const success = launchedProcess.kill('SIGINT'); + const success = launchedProcess.kill(signal); if (!success) throw new Error(`SIGINT didn't succeed, fall back to non-graceful shutdown`); - await Promise.race([ - timers.setTimeout(this._options.shutdownTimeout ?? 500).then(() => { - // @ts-expect-error. SIGINT didn't kill the process, but `processLauncher` will only attempt killing it if this is false - launchedProcess.killed = false; - return Promise.reject(new Error(`server didn't close gracefully within a second, falling back to non-graceful shutdown`)); - }), - new Promise(f => launchedProcess.once('exit', f)), - ]); + + const processExit = new Promise(f => launchedProcess.once('exit', f)); + if (timeout === 0) { + await processExit; + } else { + await Promise.race([ + processExit, + timers.setTimeout(timeout).then(() => { + // @ts-expect-error. SIGINT didn't kill the process, but `processLauncher` will only attempt killing it if this is false + launchedProcess.killed = false; + return Promise.reject(new Error(`process didn't close gracefully within timeout, falling back to SIGKILL`)); + }), + ]); + } }, log: () => {}, onExit: code => processExitedReject(new Error(code ? `Process from config.webServer was not able to start. Exit code: ${code}` : 'Process from config.webServer exited early.')), diff --git a/packages/playwright/types/test.d.ts b/packages/playwright/types/test.d.ts index 2f0ba7e394..3ea1f96bad 100644 --- a/packages/playwright/types/test.d.ts +++ b/packages/playwright/types/test.d.ts @@ -9372,10 +9372,12 @@ interface TestConfigWebServer { timeout?: number; /** - * How long to wait for the process to gracefully shut down after it was sent `SIGINT`. If exceeded, process is killed - * with `SIGKILL`. Defaults to 500 milliseconds. + * How to shut down the process gracefully. If unspecified, the process group is forcefully `SIGKILL`ed. If set to `{ + * SIGINT: 500 }`, the top process is sent a `SIGINT` signal, followed by `SIGKILL` if it doesn't exit within 500ms. + * You can also use `SIGTERM` instead. A `0` timeout means no `SIGKILL` will be sent. Windows doesn't support `SIGINT` + * and `SIGTERM` signals, so this option is ignored. */ - shutdownTimeout?: number; + kill?: { SIGINT: number }|{ SIGTERM: number }; /** * The url on your http server that is expected to return a 2xx, 3xx, 400, 401, 402, or 403 status code when the diff --git a/tests/playwright-test/web-server.spec.ts b/tests/playwright-test/web-server.spec.ts index aa447f9a3f..f3c3d772f0 100644 --- a/tests/playwright-test/web-server.spec.ts +++ b/tests/playwright-test/web-server.spec.ts @@ -745,7 +745,7 @@ test('should forward stdout when set to "pipe" before server is ready', async ({ expect(result.output).not.toContain('Timed out waiting 3000ms'); }); -test.describe('graceful shutdown', () => { +test.describe('kill option', () => { test.skip(process.platform === 'win32', 'No sending SIGINT on Windows'); const files = (additionalOptions = {}) => { @@ -753,6 +753,7 @@ test.describe('graceful shutdown', () => { return { 'web-server.js': ` process.on('SIGINT', () => { console.log('%%webserver received SIGINT but stubbornly refuses to wind down') }) + process.on('SIGTERM', () => { console.log('%%webserver received SIGTERM but stubbornly refuses to wind down') }) const server = require('http').createServer((req, res) => { res.end("ok"); }) server.listen(process.argv[2], () => { console.log('webserver started'); }); `, @@ -774,23 +775,33 @@ test.describe('graceful shutdown', () => { }; }; - test('sends SIGINT by default', async ({ interactWithTestRunner }) => { + test('sends SIGKILL by default', async ({ interactWithTestRunner }) => { const testProcess = await interactWithTestRunner(files(), { workers: 1 }); await testProcess.waitForOutput('webserver started'); process.kill(testProcess.process.pid!, 'SIGINT'); await testProcess.exited; - expect(testProcess.outputLines({ prefix: '[WebServer] ' })).toEqual(['webserver received SIGINT but stubbornly refuses to wind down']); - }); - - test('can be disabled', async ({ interactWithTestRunner }) => { - const testProcess = await interactWithTestRunner(files({ shutdownTimeout: 0 }), { workers: 1 }); - - await testProcess.waitForOutput('webserver started'); - process.kill(testProcess.process.pid!, 'SIGINT'); - await testProcess.exited; - expect(testProcess.outputLines({ prefix: '[WebServer] ' })).toEqual([]); }); + + test('can be configured to send SIGTERM', async ({ interactWithTestRunner }) => { + const testProcess = await interactWithTestRunner(files({ kill: { SIGTERM: 500 } }), { workers: 1 }); + + await testProcess.waitForOutput('webserver started'); + process.kill(testProcess.process.pid!, 'SIGINT'); + await testProcess.exited; + + expect(testProcess.outputLines({ prefix: '[WebServer] ' })).toEqual(['webserver received SIGTERM but stubbornly refuses to wind down']); + }); + + test('can be configured to send SIGINT', async ({ interactWithTestRunner }) => { + const testProcess = await interactWithTestRunner(files({ kill: { SIGINT: 500 } }), { workers: 1 }); + + await testProcess.waitForOutput('webserver started'); + process.kill(testProcess.process.pid!, 'SIGINT'); + await testProcess.exited; + + expect(testProcess.outputLines({ prefix: '[WebServer] ' })).toEqual(['webserver received SIGINT but stubbornly refuses to wind down']); + }); });