From dfcd2a273ded64f9882d36892e1a491799c563bf Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Tue, 13 Sep 2022 17:05:37 -0700 Subject: [PATCH] fix(docker): do not pollute stdout when used with JSON reporter (#17315) This patch moves parts of docker configuration to a plugin so that it can rely on repoter for stdout. Drive-by: provide all plugins with reporter in the `setup` callback. --- packages/playwright-test/src/cli.ts | 5 +- packages/playwright-test/src/docker/docker.ts | 53 +++++++++++-------- packages/playwright-test/src/plugins/index.ts | 15 +++--- .../src/plugins/webServerPlugin.ts | 31 +++++------ packages/playwright-test/src/runner.ts | 8 ++- 5 files changed, 65 insertions(+), 47 deletions(-) diff --git a/packages/playwright-test/src/cli.ts b/packages/playwright-test/src/cli.ts index a158ca2753..00a370bafe 100644 --- a/packages/playwright-test/src/cli.ts +++ b/packages/playwright-test/src/cli.ts @@ -100,9 +100,8 @@ function addTestCommand(program: Command, isDocker: boolean) { command.option('-x', `Stop after the first failure`); command.action(async (args, opts) => { try { - isDocker = isDocker || !!process.env.PLAYWRIGHT_DOCKER; - if (isDocker && !process.env.PW_TS_ESM_ON) - await docker.configureTestRunnerToUseDocker(); + if (isDocker) + process.env.PLAYWRIGHT_DOCKER = '1'; await runTests(args, opts); } catch (e) { console.error(e); diff --git a/packages/playwright-test/src/docker/docker.ts b/packages/playwright-test/src/docker/docker.ts index b86dab8a56..df0dbf9622 100644 --- a/packages/playwright-test/src/docker/docker.ts +++ b/packages/playwright-test/src/docker/docker.ts @@ -22,6 +22,8 @@ import { spawnAsync } from 'playwright-core/lib/utils/spawnAsync'; import * as utils from 'playwright-core/lib/utils'; import { getPlaywrightVersion } from 'playwright-core/lib/common/userAgent'; import * as dockerApi from './dockerApi'; +import type { TestRunnerPlugin } from '../plugins'; +import type { FullConfig, Reporter, Suite } from '../../types/testReporter'; const VRT_IMAGE_DISTRO = 'focal'; const VRT_IMAGE_NAME = `playwright:local-${getPlaywrightVersion()}-${VRT_IMAGE_DISTRO}`; @@ -128,28 +130,37 @@ export async function buildPlaywrightImage() { console.log(`Done!`); } -export async function configureTestRunnerToUseDocker() { - console.log(colors.dim('Using docker container to run browsers.')); - await checkDockerEngineIsRunningOrDie(); - let info = await containerInfo(); - if (!info) { - process.stdout.write(colors.dim(`Starting docker container... `)); - const time = Date.now(); - info = await ensurePlaywrightContainerOrDie(); - const deltaMs = (Date.now() - time); - console.log(colors.dim('Done in ' + (deltaMs / 1000).toFixed(1) + 's')); - console.log(colors.dim('The Docker container will keep running after tests finished.')); - console.log(colors.dim('Stop manually using:')); - console.log(colors.dim(' npx playwright docker stop')); - } - console.log(colors.dim(`View screen: ${info.vncSession}`)); - process.env.PW_TEST_CONNECT_WS_ENDPOINT = info.wsEndpoint; - process.env.PW_TEST_CONNECT_HEADERS = JSON.stringify({ - 'x-playwright-proxy': '*', - }); - process.env.PLAYWRIGHT_DOCKER = '1'; -} +export const dockerPlugin: TestRunnerPlugin = { + name: 'playwright:docker', + async setup(config: FullConfig, configDir: string, rootSuite: Suite, reporter: Reporter) { + if (!process.env.PLAYWRIGHT_DOCKER) + return; + + const print = (text: string) => reporter.onStdOut?.(text); + const println = (text: string) => reporter.onStdOut?.(text + '\n'); + + println(colors.dim('Using docker container to run browsers.')); + await checkDockerEngineIsRunningOrDie(); + let info = await containerInfo(); + if (!info) { + print(colors.dim(`Starting docker container... `)); + const time = Date.now(); + info = await ensurePlaywrightContainerOrDie(); + const deltaMs = (Date.now() - time); + println(colors.dim('Done in ' + (deltaMs / 1000).toFixed(1) + 's')); + println(colors.dim('The Docker container will keep running after tests finished.')); + println(colors.dim('Stop manually using:')); + println(colors.dim(' npx playwright docker stop')); + } + println(colors.dim(`View screen: ${info.vncSession}`)); + println(''); + process.env.PW_TEST_CONNECT_WS_ENDPOINT = info.wsEndpoint; + process.env.PW_TEST_CONNECT_HEADERS = JSON.stringify({ + 'x-playwright-proxy': '*', + }); + }, +}; interface ContainerInfo { wsEndpoint: string; diff --git a/packages/playwright-test/src/plugins/index.ts b/packages/playwright-test/src/plugins/index.ts index b43a040058..7255604749 100644 --- a/packages/playwright-test/src/plugins/index.ts +++ b/packages/playwright-test/src/plugins/index.ts @@ -14,13 +14,13 @@ * limitations under the License. */ -import type { Suite } from '../../types/testReporter'; +import type { Suite, Reporter } from '../../types/testReporter'; import type { Runner } from '../runner'; import type { FullConfig } from '../types'; export interface TestRunnerPlugin { name: string; - setup?(config: FullConfig, configDir: string, rootSuite: Suite): Promise; + setup?(config: FullConfig, configDir: string, rootSuite: Suite, reporter: Reporter): Promise; teardown?(): Promise; } @@ -28,15 +28,18 @@ export { webServer } from './webServerPlugin'; export { gitCommitInfo } from './gitCommitInfoPlugin'; let runnerInstanceToAddPluginsTo: Runner | undefined; +const deferredPlugins: TestRunnerPlugin[] = []; export const setRunnerToAddPluginsTo = (runner: Runner) => { runnerInstanceToAddPluginsTo = runner; + for (const plugin of deferredPlugins) + runnerInstanceToAddPluginsTo.addPlugin(plugin); }; export const addRunnerPlugin = (plugin: TestRunnerPlugin | (() => TestRunnerPlugin)) => { - // Only present in runner, absent in worker. - if (runnerInstanceToAddPluginsTo) { - plugin = typeof plugin === 'function' ? plugin() : plugin; + plugin = typeof plugin === 'function' ? plugin() : plugin; + if (runnerInstanceToAddPluginsTo) runnerInstanceToAddPluginsTo.addPlugin(plugin); - } + else + deferredPlugins.push(plugin); }; diff --git a/packages/playwright-test/src/plugins/webServerPlugin.ts b/packages/playwright-test/src/plugins/webServerPlugin.ts index 4ce87a6de8..6a3f5b5b97 100644 --- a/packages/playwright-test/src/plugins/webServerPlugin.ts +++ b/packages/playwright-test/src/plugins/webServerPlugin.ts @@ -22,7 +22,7 @@ import { debug } from 'playwright-core/lib/utilsBundle'; import { raceAgainstTimeout } from 'playwright-core/lib/utils/timeoutRunner'; import { launchProcess } from 'playwright-core/lib/utils/processLauncher'; -import type { FullConfig, Reporter } from '../../types/testReporter'; +import type { FullConfig, Reporter, Suite } from '../../types/testReporter'; import type { TestRunnerPlugin } from '.'; import type { FullConfigInternal } from '../types'; import { envWithoutExperimentalLoaderOptions } from '../cli'; @@ -45,21 +45,22 @@ const DEFAULT_ENVIRONMENT_VARIABLES = { const debugWebServer = debug('pw:webserver'); export class WebServerPlugin implements TestRunnerPlugin { - private _isAvailable: () => Promise; + private _isAvailable?: () => Promise; private _killProcess?: () => Promise; private _processExitedPromise!: Promise; private _options: WebServerPluginOptions; - private _reporter: Reporter; + private _checkPortOnly: boolean; + private _reporter?: Reporter; name = 'playwright:webserver'; - constructor(options: WebServerPluginOptions, checkPortOnly: boolean, reporter: Reporter) { - this._reporter = reporter; + constructor(options: WebServerPluginOptions, checkPortOnly: boolean) { this._options = options; - this._isAvailable = getIsAvailableFunction(options.url, checkPortOnly, !!options.ignoreHTTPSErrors, this._reporter.onStdErr?.bind(this._reporter)); + this._checkPortOnly = checkPortOnly; } - - public async setup(config: FullConfig, configDir: string) { + public async setup(config: FullConfig, configDir: string, rootSuite: Suite, reporter: Reporter) { + this._reporter = reporter; + this._isAvailable = getIsAvailableFunction(this._options.url, this._checkPortOnly, !!this._options.ignoreHTTPSErrors, this._reporter.onStdErr?.bind(this._reporter)); this._options.cwd = this._options.cwd ? path.resolve(configDir, this._options.cwd) : configDir; try { await this._startProcess(); @@ -78,7 +79,7 @@ export class WebServerPlugin implements TestRunnerPlugin { let processExitedReject = (error: Error) => { }; this._processExitedPromise = new Promise((_, reject) => processExitedReject = reject); - const isAlreadyAvailable = await this._isAvailable(); + const isAlreadyAvailable = await this._isAvailable!(); if (isAlreadyAvailable) { debugWebServer(`WebServer is already available`); if (this._options.reuseExistingServer) @@ -107,10 +108,10 @@ export class WebServerPlugin implements TestRunnerPlugin { debugWebServer(`Process started`); - launchedProcess.stderr!.on('data', line => this._reporter.onStdErr?.('[WebServer] ' + line.toString())); + launchedProcess.stderr!.on('data', line => this._reporter!.onStdErr?.('[WebServer] ' + line.toString())); launchedProcess.stdout!.on('data', line => { if (debugWebServer.enabled) - this._reporter.onStdOut?.('[WebServer] ' + line.toString()); + this._reporter!.onStdOut?.('[WebServer] ' + line.toString()); }); } @@ -124,7 +125,7 @@ export class WebServerPlugin implements TestRunnerPlugin { const launchTimeout = this._options.timeout || 60 * 1000; const cancellationToken = { canceled: false }; const { timedOut } = (await Promise.race([ - raceAgainstTimeout(() => waitFor(this._isAvailable, cancellationToken), launchTimeout), + raceAgainstTimeout(() => waitFor(this._isAvailable!, cancellationToken), launchTimeout), this._processExitedPromise, ])); cancellationToken.canceled = true; @@ -203,10 +204,10 @@ function getIsAvailableFunction(url: string, checkPortOnly: boolean, ignoreHTTPS export const webServer = (options: WebServerPluginOptions): TestRunnerPlugin => { // eslint-disable-next-line no-console - return new WebServerPlugin(options, false, { onStdOut: d => console.log(d.toString()), onStdErr: d => console.error(d.toString()) }); + return new WebServerPlugin(options, false); }; -export const webServerPluginsForConfig = (config: FullConfigInternal, reporter: Reporter): TestRunnerPlugin[] => { +export const webServerPluginsForConfig = (config: FullConfigInternal): TestRunnerPlugin[] => { const shouldSetBaseUrl = !!config.webServer; const webServerPlugins = []; for (const webServerConfig of config._webServers) { @@ -219,7 +220,7 @@ export const webServerPluginsForConfig = (config: FullConfigInternal, reporter: if (shouldSetBaseUrl && !webServerConfig.url) process.env.PLAYWRIGHT_TEST_BASE_URL = url; - webServerPlugins.push(new WebServerPlugin({ ...webServerConfig, url }, webServerConfig.port !== undefined, reporter)); + webServerPlugins.push(new WebServerPlugin({ ...webServerConfig, url }, webServerConfig.port !== undefined)); } return webServerPlugins; diff --git a/packages/playwright-test/src/runner.ts b/packages/playwright-test/src/runner.ts index c783afae9d..6cd71b5016 100644 --- a/packages/playwright-test/src/runner.ts +++ b/packages/playwright-test/src/runner.ts @@ -44,6 +44,7 @@ import { SigIntWatcher } from './sigIntWatcher'; import type { TestRunnerPlugin } from './plugins'; import { setRunnerToAddPluginsTo } from './plugins'; import { webServerPluginsForConfig } from './plugins/webServerPlugin'; +import { dockerPlugin } from './docker/docker'; import { MultiMap } from 'playwright-core/lib/utils/multimap'; const removeFolderAsync = promisify(rimraf); @@ -603,14 +604,17 @@ export class Runner { }; // Legacy webServer support. - this._plugins.push(...webServerPluginsForConfig(config, this._reporter)); + this._plugins.push(...webServerPluginsForConfig(config)); + + // Docker support. + this._plugins.push(dockerPlugin); await this._runAndReportError(async () => { // First run the plugins, if plugin is a web server we want it to run before the // config's global setup. for (const plugin of this._plugins) { await Promise.race([ - plugin.setup?.(config, config._configDir, rootSuite), + plugin.setup?.(config, config._configDir, rootSuite, this._reporter), sigintWatcher.promise(), ]); if (sigintWatcher.hadSignal())