From 9f31bcfbab7405fec671c11dc6661fe9e041011c Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Sun, 22 Jan 2023 15:04:29 -0800 Subject: [PATCH] chore: refactor graceful close, rename to host/main (#20283) --- packages/playwright-test/src/loaderHost.ts | 4 ++-- .../src/{loaderRunner.ts => loaderMain.ts} | 4 ++-- packages/playwright-test/src/process.ts | 22 ++++------------- packages/playwright-test/src/processHost.ts | 4 ++-- packages/playwright-test/src/runner.ts | 2 +- packages/playwright-test/src/workerHost.ts | 4 ++-- .../src/{workerRunner.ts => workerMain.ts} | 20 +++++++++------- tests/playwright-test/basic.spec.ts | 2 +- tests/playwright-test/esm.spec.ts | 5 +++- tests/playwright-test/loader.spec.ts | 4 ++++ tests/playwright-test/playwright.config.ts | 1 - .../playwright.ct-build.spec.ts | 24 +++++++++++++++---- .../playwright.ct-react.spec.ts | 16 +++++++++++++ 13 files changed, 71 insertions(+), 41 deletions(-) rename packages/playwright-test/src/{loaderRunner.ts => loaderMain.ts} (92%) rename packages/playwright-test/src/{workerRunner.ts => workerMain.ts} (98%) diff --git a/packages/playwright-test/src/loaderHost.ts b/packages/playwright-test/src/loaderHost.ts index d2bdfd3c90..2b4220764d 100644 --- a/packages/playwright-test/src/loaderHost.ts +++ b/packages/playwright-test/src/loaderHost.ts @@ -19,9 +19,9 @@ import type { SerializedConfig } from './ipc'; import { ProcessHost } from './processHost'; import { Suite } from './test'; -export class LoaderHost extends ProcessHost { +export class LoaderHost extends ProcessHost { constructor() { - super(require.resolve('./loaderRunner.js'), 'loader'); + super(require.resolve('./loaderMain.js'), 'loader'); } async start(config: SerializedConfig) { diff --git a/packages/playwright-test/src/loaderRunner.ts b/packages/playwright-test/src/loaderMain.ts similarity index 92% rename from packages/playwright-test/src/loaderRunner.ts rename to packages/playwright-test/src/loaderMain.ts index 7157945049..bc6dd88293 100644 --- a/packages/playwright-test/src/loaderRunner.ts +++ b/packages/playwright-test/src/loaderMain.ts @@ -21,7 +21,7 @@ import { ProcessRunner } from './process'; import { loadTestFilesInProcess } from './testLoader'; import { setFatalErrorSink } from './globals'; -export class LoaderRunner extends ProcessRunner { +export class LoaderMain extends ProcessRunner { private _config: SerializedConfig; private _configLoaderPromise: Promise | undefined; @@ -45,4 +45,4 @@ export class LoaderRunner extends ProcessRunner { } } -export const create = (config: SerializedConfig) => new LoaderRunner(config); +export const create = (config: SerializedConfig) => new LoaderMain(config); diff --git a/packages/playwright-test/src/process.ts b/packages/playwright-test/src/process.ts index aededb85f6..0d8f990707 100644 --- a/packages/playwright-test/src/process.ts +++ b/packages/playwright-test/src/process.ts @@ -15,7 +15,7 @@ */ import type { WriteStream } from 'tty'; -import type { ProcessInitParams, TeardownErrorsPayload, TtyParams } from './ipc'; +import type { ProcessInitParams, TtyParams } from './ipc'; import { startProfiling, stopProfiling } from './profiler'; import type { TestInfoError } from './types'; import { serializeError } from './util'; @@ -35,7 +35,6 @@ export type ProtocolResponse = { }; export class ProcessRunner { - appendProcessTeardownDiagnostics(error: TestInfoError) { } async gracefullyClose(): Promise { } protected dispatchEvent(method: string, params: any) { @@ -53,7 +52,7 @@ process.on('SIGINT', () => {}); process.on('SIGTERM', () => {}); let processRunner: ProcessRunner; -let processName: string | undefined; +let processName: string; process.on('message', async message => { if (message.method === '__init__') { const { processParams, runnerParams, runnerScript } = message.params as { processParams: ProcessInitParams, runnerParams: any, runnerScript: string }; @@ -62,6 +61,7 @@ process.on('message', async message => { startProfiling(); const { create } = require(runnerScript); processRunner = create(runnerParams) as ProcessRunner; + processName = processParams.processName; return; } if (message.method === '__stop__') { @@ -88,20 +88,8 @@ async function gracefullyCloseAndExit() { // Force exit after 30 seconds. setTimeout(() => process.exit(0), 30000); // Meanwhile, try to gracefully shutdown. - try { - if (processRunner) - await processRunner.gracefullyClose(); - if (processName) - await stopProfiling(processName); - } catch (e) { - try { - const error = serializeError(e); - processRunner.appendProcessTeardownDiagnostics(error); - const payload: TeardownErrorsPayload = { fatalErrors: [error] }; - sendMessageToParent({ method: 'teardownErrors', params: payload }); - } catch { - } - } + await processRunner.gracefullyClose().catch(() => {}); + await stopProfiling(processName).catch(() => {}); process.exit(0); } diff --git a/packages/playwright-test/src/processHost.ts b/packages/playwright-test/src/processHost.ts index 200a98acdc..92678f8ad0 100644 --- a/packages/playwright-test/src/processHost.ts +++ b/packages/playwright-test/src/processHost.ts @@ -26,7 +26,7 @@ export type ProcessExitData = { signal: NodeJS.Signals | null; }; -export class ProcessHost extends EventEmitter { +export class ProcessHost extends EventEmitter { private process!: child_process.ChildProcess; private _didSendStop = false; private _didFail = false; @@ -42,7 +42,7 @@ export class ProcessHost extends EventEmitter { this._processName = processName; } - protected async startRunner(runnerParams: InitParams, inheritStdio: boolean, env: NodeJS.ProcessEnv) { + protected async startRunner(runnerParams: any, inheritStdio: boolean, env: NodeJS.ProcessEnv) { this.process = child_process.fork(require.resolve('./process'), { detached: false, env: { ...process.env, ...env }, diff --git a/packages/playwright-test/src/runner.ts b/packages/playwright-test/src/runner.ts index 80c1306818..d1b37755a1 100644 --- a/packages/playwright-test/src/runner.ts +++ b/packages/playwright-test/src/runner.ts @@ -341,7 +341,7 @@ export class Runner { } private async _loadTests(testFiles: Set): Promise { - if (process.env.PWTEST_OOP_LOADER) { + if (process.env.PW_TEST_OOP_LOADER) { const loaderHost = new LoaderHost(); await loaderHost.start(this._configLoader.serializedConfig()); try { diff --git a/packages/playwright-test/src/workerHost.ts b/packages/playwright-test/src/workerHost.ts index 19ddff60ff..c36f5897d4 100644 --- a/packages/playwright-test/src/workerHost.ts +++ b/packages/playwright-test/src/workerHost.ts @@ -20,7 +20,7 @@ import { ProcessHost } from './processHost'; let lastWorkerIndex = 0; -export class WorkerHost extends ProcessHost { +export class WorkerHost extends ProcessHost { readonly parallelIndex: number; readonly workerIndex: number; private _hash: string; @@ -29,7 +29,7 @@ export class WorkerHost extends ProcessHost { constructor(testGroup: TestGroup, parallelIndex: number, config: SerializedConfig) { const workerIndex = lastWorkerIndex++; - super(require.resolve('./workerRunner.js'), `worker-${workerIndex}`); + super(require.resolve('./workerMain.js'), `worker-${workerIndex}`); this.workerIndex = workerIndex; this.parallelIndex = parallelIndex; this._hash = testGroup.workerHash; diff --git a/packages/playwright-test/src/workerRunner.ts b/packages/playwright-test/src/workerMain.ts similarity index 98% rename from packages/playwright-test/src/workerRunner.ts rename to packages/playwright-test/src/workerMain.ts index 2329810c2c..559c7bed93 100644 --- a/packages/playwright-test/src/workerRunner.ts +++ b/packages/playwright-test/src/workerMain.ts @@ -34,7 +34,7 @@ import { PoolBuilder } from './poolBuilder'; const removeFolderAsync = util.promisify(rimraf); -export class WorkerRunner extends ProcessRunner { +export class WorkerMain extends ProcessRunner { private _params: WorkerInitParams; private _configLoader!: ConfigLoader; private _testLoader!: TestLoader; @@ -109,19 +109,23 @@ export class WorkerRunner extends ProcessRunner { } override async gracefullyClose() { - await this._stop(); + try { + await this._stop(); + // We have to load the project to get the right deadline below. + await this._loadIfNeeded(); + await this._teardownScopes(); + } catch (e) { + this._fatalErrors.push(serializeError(e)); + } - // We have to load the project to get the right deadline below. - await this._loadIfNeeded(); - await this._teardownScopes(); if (this._fatalErrors.length) { - this.appendProcessTeardownDiagnostics(this._fatalErrors[this._fatalErrors.length - 1]); + this._appendProcessTeardownDiagnostics(this._fatalErrors[this._fatalErrors.length - 1]); const payload: TeardownErrorsPayload = { fatalErrors: this._fatalErrors }; this.dispatchEvent('teardownErrors', payload); } } - override appendProcessTeardownDiagnostics(error: TestInfoError) { + private _appendProcessTeardownDiagnostics(error: TestInfoError) { if (!this._lastRunningTests.length) return; const count = this._totalRunningTests === 1 ? '1 test' : `${this._totalRunningTests} tests`; @@ -643,4 +647,4 @@ function chunkToParams(chunk: Buffer | string): { text?: string, buffer?: strin return { text: chunk }; } -export const create = (params: WorkerInitParams) => new WorkerRunner(params); +export const create = (params: WorkerInitParams) => new WorkerMain(params); diff --git a/tests/playwright-test/basic.spec.ts b/tests/playwright-test/basic.spec.ts index c6c8bbdab3..56d8ba822f 100644 --- a/tests/playwright-test/basic.spec.ts +++ b/tests/playwright-test/basic.spec.ts @@ -63,7 +63,7 @@ test('should succeed', async ({ runInlineTest }) => { test('should report suite errors', async ({ runInlineTest }) => { const { exitCode, failed, output } = await runInlineTest({ 'suite-error.spec.js': ` - if (new Error().stack.includes('workerRunner')) + if (new Error().stack.includes('workerMain')) throw new Error('Suite error'); const { test } = pwt; diff --git a/tests/playwright-test/esm.spec.ts b/tests/playwright-test/esm.spec.ts index a0fa8bebdd..ef77cdd411 100644 --- a/tests/playwright-test/esm.spec.ts +++ b/tests/playwright-test/esm.spec.ts @@ -280,7 +280,10 @@ test('should resolve .js import to .tsx file in ESM mode for components', async test.skip(nodeVersion.major < 16); const result = await runInlineTest({ 'package.json': `{ "type": "module" }`, - 'playwright.config.ts': `export default { projects: [{name: 'foo'}] };`, + 'playwright.config.ts': ` + import { defineConfig } from '@playwright/experimental-ct-react'; + export default defineConfig({ projects: [{name: 'foo'}] }); + `, 'playwright/index.html': ``, 'playwright/index.ts': ``, diff --git a/tests/playwright-test/loader.spec.ts b/tests/playwright-test/loader.spec.ts index d25cd5b669..a254a99dad 100644 --- a/tests/playwright-test/loader.spec.ts +++ b/tests/playwright-test/loader.spec.ts @@ -563,6 +563,10 @@ test('should resolve .js import to .tsx file in non-ESM mode', async ({ runInlin test('should resolve .js import to .tsx file in non-ESM mode for components', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': ` + import { defineConfig } from '@playwright/experimental-ct-react'; + export default defineConfig({ projects: [{name: 'foo'}] }); + `, 'playwright/index.html': ``, 'playwright/index.ts': ``, diff --git a/tests/playwright-test/playwright.config.ts b/tests/playwright-test/playwright.config.ts index f342a50851..8aa62bb688 100644 --- a/tests/playwright-test/playwright.config.ts +++ b/tests/playwright-test/playwright.config.ts @@ -24,7 +24,6 @@ const outputDir = path.join(__dirname, '..', '..', 'test-results'); const config: Config = { timeout: 30000, forbidOnly: !!process.env.CI, - fullyParallel: !process.env.CI, workers: process.env.CI ? 2 : undefined, preserveOutput: process.env.CI ? 'failures-only' : 'always', snapshotPathTemplate: '__screenshots__/{testFilePath}/{arg}{ext}', diff --git a/tests/playwright-test/playwright.ct-build.spec.ts b/tests/playwright-test/playwright.ct-build.spec.ts index b3a892eecb..a2f2a0e7e0 100644 --- a/tests/playwright-test/playwright.ct-build.spec.ts +++ b/tests/playwright-test/playwright.ct-build.spec.ts @@ -19,8 +19,14 @@ import fs from 'fs'; test.describe.configure({ mode: 'parallel' }); +const playwrightConfig = ` + import { defineConfig } from '@playwright/experimental-ct-react'; + export default defineConfig({ projects: [{name: 'foo'}] }); +`; + test('should work with the empty component list', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.js': ``, @@ -45,6 +51,7 @@ test('should work with the empty component list', async ({ runInlineTest }, test test('should extract component list', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.ts': ``, @@ -206,6 +213,7 @@ test('should cache build', async ({ runInlineTest }, testInfo) => { await test.step('original test', async () => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.ts': ``, @@ -232,7 +240,9 @@ test('should cache build', async ({ runInlineTest }, testInfo) => { }); await test.step('re-run same test', async () => { - const result = await runInlineTest({}, { workers: 1 }); + const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, + }, { workers: 1 }); expect(result.exitCode).toBe(0); expect(result.passed).toBe(1); const output = stripAnsi(result.output); @@ -241,6 +251,7 @@ test('should cache build', async ({ runInlineTest }, testInfo) => { await test.step('modify test', async () => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'src/button.test.tsx': ` //@no-header import { test, expect } from '@playwright/experimental-ct-react'; @@ -260,6 +271,7 @@ test('should cache build', async ({ runInlineTest }, testInfo) => { await test.step('modify source', async () => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'src/button.tsx': ` export const Button = () => ; `, @@ -273,6 +285,7 @@ test('should cache build', async ({ runInlineTest }, testInfo) => { test('should not use global config for preview', async ({ runInlineTest }) => { const result1 = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.js': ``, 'vite.config.js': ` @@ -293,7 +306,9 @@ test('should not use global config for preview', async ({ runInlineTest }) => { expect(result1.exitCode).toBe(0); expect(result1.passed).toBe(1); - const result2 = await runInlineTest({}, { workers: 1 }); + const result2 = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, + }, { workers: 1 }); expect(result2.exitCode).toBe(0); expect(result2.passed).toBe(1); }); @@ -304,8 +319,9 @@ test('should work with https enabled', async ({ runInlineTest }) => { 'playwright/index.js': `//@no-header`, 'playwright.config.js': ` //@no-header + import { defineConfig } from '@playwright/experimental-ct-react'; import basicSsl from '@vitejs/plugin-basic-ssl'; - export default { + export default defineConfig({ use: { ignoreHTTPSErrors: true, ctViteConfig: { @@ -315,7 +331,7 @@ test('should work with https enabled', async ({ runInlineTest }) => { } } }, - }; + }); `, 'http.test.tsx': ` //@no-header diff --git a/tests/playwright-test/playwright.ct-react.spec.ts b/tests/playwright-test/playwright.ct-react.spec.ts index 16a0b7c39f..e3365e71e0 100644 --- a/tests/playwright-test/playwright.ct-react.spec.ts +++ b/tests/playwright-test/playwright.ct-react.spec.ts @@ -16,8 +16,14 @@ import { test, expect } from './playwright-test-fixtures'; +const playwrightConfig = ` + import { defineConfig } from '@playwright/experimental-ct-react'; + export default defineConfig({ projects: [{name: 'foo'}] }); +`; + test('should work with TSX', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.ts': ` //@no-header @@ -44,6 +50,7 @@ test('should work with TSX', async ({ runInlineTest }) => { test('should work with JSX', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.js': ` //@no-header @@ -72,6 +79,7 @@ test('should work with JSX', async ({ runInlineTest }) => { test('should work with JSX in JS', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.js': ` //@no-header @@ -100,6 +108,7 @@ test('should work with JSX in JS', async ({ runInlineTest }) => { test('should work with JSX in JS and in JSX', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.js': ` //@no-header @@ -140,6 +149,7 @@ test('should work with JSX in JS and in JSX', async ({ runInlineTest }) => { test('should work with stray TSX import', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.ts': ` //@no-header @@ -174,6 +184,7 @@ test('should work with stray TSX import', async ({ runInlineTest }) => { test('should work with stray JSX import', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.js': ` //@no-header @@ -208,6 +219,7 @@ test('should work with stray JSX import', async ({ runInlineTest }) => { test.fixme('should work with stray JS import', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.js': ` //@no-header @@ -242,6 +254,7 @@ test.fixme('should work with stray JS import', async ({ runInlineTest }) => { test('should work with JSX in variable', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.js': ` //@no-header @@ -272,6 +285,7 @@ test('should work with JSX in variable', async ({ runInlineTest }) => { test('should return root locator for fragments', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.js': `//@no-header`, @@ -299,6 +313,7 @@ test('should return root locator for fragments', async ({ runInlineTest }) => { test('should respect default property values', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.ts': `//@no-header`, 'src/label.tsx': `//@no-header @@ -323,6 +338,7 @@ test('should respect default property values', async ({ runInlineTest }) => { test('should bundle public folder', async ({ runInlineTest }) => { const result = await runInlineTest({ + 'playwright.config.ts': playwrightConfig, 'playwright/index.html': ``, 'playwright/index.ts': ` //@no-header