From 7a093329fa50747cdc7528eb34f3e0735e2c6cc9 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 8 Feb 2023 12:44:51 -0800 Subject: [PATCH] chore: do not run all on watch (#20758) --- packages/playwright-test/src/cli.ts | 3 +- packages/playwright-test/src/runner/runner.ts | 18 +-- .../playwright-test/src/runner/taskRunner.ts | 104 +++++++++--------- packages/playwright-test/src/runner/tasks.ts | 39 ++++--- .../playwright-test/src/runner/watchMode.ts | 37 +++++-- tests/playwright-test/global-setup.spec.ts | 2 +- 6 files changed, 118 insertions(+), 85 deletions(-) diff --git a/packages/playwright-test/src/cli.ts b/packages/playwright-test/src/cli.ts index 7f19b3f40f..ad1471e5fc 100644 --- a/packages/playwright-test/src/cli.ts +++ b/packages/playwright-test/src/cli.ts @@ -167,9 +167,8 @@ async function runTests(args: string[], opts: { [key: string]: any }) { config._internal.passWithNoTests = !!opts.passWithNoTests; const runner = new Runner(config); - const status = await runner.runAllTests(!!opts.watch); + const status = opts.watch ? await runner.watchAllTests() : await runner.runAllTests(); await stopProfiling(undefined); - if (status === 'interrupted') process.exit(130); process.exit(status === 'passed' ? 0 : 1); diff --git a/packages/playwright-test/src/runner/runner.ts b/packages/playwright-test/src/runner/runner.ts index 90ccfa461d..0cb9656e11 100644 --- a/packages/playwright-test/src/runner/runner.ts +++ b/packages/playwright-test/src/runner/runner.ts @@ -47,7 +47,7 @@ export class Runner { return report; } - async runAllTests(watchMode: boolean): Promise { + async runAllTests(): Promise { const config = this._config; const listOnly = config._internal.listOnly; const deadline = config.globalTimeout ? monotonicTime() + config.globalTimeout : 0; @@ -55,9 +55,9 @@ export class Runner { // Legacy webServer support. webServerPluginsForConfig(config).forEach(p => config._internal.plugins.push({ factory: p })); - const reporter = await createReporter(config, listOnly ? 'list' : watchMode ? 'watch' : 'run'); + const reporter = await createReporter(config, listOnly ? 'list' : 'run'); const taskRunner = listOnly ? createTaskRunnerForList(config, reporter) - : createTaskRunner(config, reporter, watchMode); + : createTaskRunner(config, reporter); const context: TaskRunnerState = { config, @@ -78,16 +78,12 @@ export class Runner { const taskStatus = await taskRunner.run(context, deadline); let status: FullResult['status'] = 'passed'; - const failedTests = context.rootSuite?.allTests().filter(test => !test.ok()) || []; - if (context.phases.find(p => p.dispatcher.hasWorkerErrors()) || failedTests.length) + if (context.phases.find(p => p.dispatcher.hasWorkerErrors()) || context.rootSuite?.allTests().some(test => !test.ok())) status = 'failed'; if (status === 'passed' && taskStatus !== 'passed') status = taskStatus; await reporter.onExit({ status }); - if (watchMode) - status = await runWatchModeLoop(config, failedTests); - // Calling process.exit() might truncate large stdout/stderr output. // See https://github.com/nodejs/node/issues/6456. // See https://github.com/nodejs/node/issues/12921 @@ -95,6 +91,12 @@ export class Runner { await new Promise(resolve => process.stderr.write('', () => resolve())); return status; } + + async watchAllTests(): Promise { + const config = this._config; + webServerPluginsForConfig(config).forEach(p => config._internal.plugins.push({ factory: p })); + return await runWatchModeLoop(config); + } } function sanitizeConfigForJSON(object: any, visited: Set): any { diff --git a/packages/playwright-test/src/runner/taskRunner.ts b/packages/playwright-test/src/runner/taskRunner.ts index cca8813c50..2638c53fcf 100644 --- a/packages/playwright-test/src/runner/taskRunner.ts +++ b/packages/playwright-test/src/runner/taskRunner.ts @@ -45,66 +45,70 @@ export class TaskRunner { } async run(context: Context, deadline: number): Promise { + const { status, cleanup } = await this.runDeferCleanup(context, deadline); + const teardownStatus = await cleanup(); + return status === 'passed' ? teardownStatus : status; + } + + async runDeferCleanup(context: Context, deadline: number): Promise<{ status: FullResult['status'], cleanup: () => Promise }> { const sigintWatcher = new SigIntWatcher(); const timeoutWatcher = new TimeoutWatcher(deadline); const teardownRunner = new TaskRunner(this._reporter, this._globalTimeoutForError); teardownRunner._isTearDown = true; - try { - let currentTaskName: string | undefined; - const taskLoop = async () => { - for (const { name, task } of this._tasks) { - currentTaskName = name; - if (this._interrupted) - break; - debug('pw:test:task')(`"${name}" started`); - const errors: TestError[] = []; - try { - const teardown = await task(context, errors); - if (teardown) - teardownRunner._tasks.unshift({ name: `teardown for ${name}`, task: teardown }); - } catch (e) { - debug('pw:test:task')(`error in "${name}": `, e); - errors.push(serializeError(e)); - } finally { - for (const error of errors) - this._reporter.onError?.(error); - if (errors.length) { - if (!this._isTearDown) - this._interrupted = true; - this._hasErrors = true; - } + let currentTaskName: string | undefined; + + const taskLoop = async () => { + for (const { name, task } of this._tasks) { + currentTaskName = name; + if (this._interrupted) + break; + debug('pw:test:task')(`"${name}" started`); + const errors: TestError[] = []; + try { + const teardown = await task(context, errors); + if (teardown) + teardownRunner._tasks.unshift({ name: `teardown for ${name}`, task: teardown }); + } catch (e) { + debug('pw:test:task')(`error in "${name}": `, e); + errors.push(serializeError(e)); + } finally { + for (const error of errors) + this._reporter.onError?.(error); + if (errors.length) { + if (!this._isTearDown) + this._interrupted = true; + this._hasErrors = true; } - debug('pw:test:task')(`"${name}" finished`); } - }; - - await Promise.race([ - taskLoop(), - sigintWatcher.promise(), - timeoutWatcher.promise, - ]); - - // Prevent subsequent tasks from running. - this._interrupted = true; - - let status: FullResult['status'] = 'passed'; - if (sigintWatcher.hadSignal()) { - status = 'interrupted'; - } else if (timeoutWatcher.timedOut()) { - this._reporter.onError?.({ message: `Timed out waiting ${this._globalTimeoutForError / 1000}s for the ${currentTaskName} to run` }); - status = 'timedout'; - } else if (this._hasErrors) { - status = 'failed'; + debug('pw:test:task')(`"${name}" finished`); } + }; - return status; - } finally { - sigintWatcher.disarm(); - timeoutWatcher.disarm(); - if (!this._isTearDown) - await teardownRunner.run(context, deadline); + await Promise.race([ + taskLoop(), + sigintWatcher.promise(), + timeoutWatcher.promise, + ]); + + sigintWatcher.disarm(); + timeoutWatcher.disarm(); + + // Prevent subsequent tasks from running. + this._interrupted = true; + + let status: FullResult['status'] = 'passed'; + if (sigintWatcher.hadSignal()) { + status = 'interrupted'; + } else if (timeoutWatcher.timedOut()) { + this._reporter.onError?.({ message: `Timed out waiting ${this._globalTimeoutForError / 1000}s for the ${currentTaskName} to run` }); + status = 'timedout'; + } else if (this._hasErrors) { + status = 'failed'; } + + const cleanup = () => teardownRunner.runDeferCleanup(context, deadline).then(r => r.status); + return { status, cleanup }; } } diff --git a/packages/playwright-test/src/runner/tasks.ts b/packages/playwright-test/src/runner/tasks.ts index 3e4625bdeb..3cee8d1ddd 100644 --- a/packages/playwright-test/src/runner/tasks.ts +++ b/packages/playwright-test/src/runner/tasks.ts @@ -50,27 +50,36 @@ export type TaskRunnerState = { }[]; }; -export function createTaskRunner(config: FullConfigInternal, reporter: Multiplexer, doNotTeardown: boolean): TaskRunner { +export function createTaskRunner(config: FullConfigInternal, reporter: Multiplexer): TaskRunner { const taskRunner = new TaskRunner(reporter, config.globalTimeout); - - for (const plugin of config._internal.plugins) - taskRunner.addTask('plugin setup', createPluginSetupTask(plugin, doNotTeardown)); - if (config.globalSetup || config.globalTeardown) - taskRunner.addTask('global setup', createGlobalSetupTask(doNotTeardown)); + addGlobalSetupTasks(taskRunner, config); taskRunner.addTask('load tests', createLoadTask('in-process')); - taskRunner.addTask('clear output', createRemoveOutputDirsTask()); - addCommonTasks(taskRunner, config); + addRunTasks(taskRunner, config); + return taskRunner; +} + +export function createTaskRunnerForWatchSetup(config: FullConfigInternal, reporter: Multiplexer): TaskRunner { + const taskRunner = new TaskRunner(reporter, 0); + addGlobalSetupTasks(taskRunner, config); return taskRunner; } export function createTaskRunnerForWatch(config: FullConfigInternal, reporter: Multiplexer, projectsToIgnore?: Set, additionalFileMatcher?: Matcher): TaskRunner { - const taskRunner = new TaskRunner(reporter, config.globalTimeout); + const taskRunner = new TaskRunner(reporter, 0); taskRunner.addTask('load tests', createLoadTask('out-of-process', projectsToIgnore, additionalFileMatcher)); - addCommonTasks(taskRunner, config); + addRunTasks(taskRunner, config); return taskRunner; } -function addCommonTasks(taskRunner: TaskRunner, config: FullConfigInternal) { +function addGlobalSetupTasks(taskRunner: TaskRunner, config: FullConfigInternal) { + for (const plugin of config._internal.plugins) + taskRunner.addTask('plugin setup', createPluginSetupTask(plugin)); + if (config.globalSetup || config.globalTeardown) + taskRunner.addTask('global setup', createGlobalSetupTask()); + taskRunner.addTask('clear output', createRemoveOutputDirsTask()); +} + +function addRunTasks(taskRunner: TaskRunner, config: FullConfigInternal) { taskRunner.addTask('create tasks', createTestGroupsTask()); taskRunner.addTask('report begin', async ({ reporter, rootSuite }) => { reporter.onBegin?.(config, rootSuite!); @@ -93,14 +102,14 @@ export function createTaskRunnerForList(config: FullConfigInternal, reporter: Mu return taskRunner; } -function createPluginSetupTask(plugin: TestRunnerPluginRegistration, doNotTeardown: boolean): Task { +function createPluginSetupTask(plugin: TestRunnerPluginRegistration): Task { return async ({ config, reporter }) => { if (typeof plugin.factory === 'function') plugin.instance = await plugin.factory(); else plugin.instance = plugin.factory; await plugin.instance?.setup?.(config, config._internal.configDir, reporter); - return doNotTeardown ? undefined : () => plugin.instance?.teardown?.(); + return () => plugin.instance?.teardown?.(); }; } @@ -111,12 +120,12 @@ function createPluginBeginTask(plugin: TestRunnerPluginRegistration): Task { +function createGlobalSetupTask(): Task { return async ({ config }) => { const setupHook = config.globalSetup ? await loadGlobalHook(config, config.globalSetup) : undefined; const teardownHook = config.globalTeardown ? await loadGlobalHook(config, config.globalTeardown) : undefined; const globalSetupResult = setupHook ? await setupHook(config) : undefined; - return doNotTeardown ? undefined : async () => { + return async () => { if (typeof globalSetupResult === 'function') await globalSetupResult(); await teardownHook?.(config); diff --git a/packages/playwright-test/src/runner/watchMode.ts b/packages/playwright-test/src/runner/watchMode.ts index 05647142e7..56cad03533 100644 --- a/packages/playwright-test/src/runner/watchMode.ts +++ b/packages/playwright-test/src/runner/watchMode.ts @@ -20,13 +20,13 @@ import type { FullConfigInternal, FullProjectInternal } from '../common/types'; import { Multiplexer } from '../reporters/multiplexer'; import { createFileMatcherFromArguments } from '../util'; import type { Matcher } from '../util'; -import { createTaskRunnerForWatch } from './tasks'; +import { createTaskRunnerForWatch, createTaskRunnerForWatchSetup } from './tasks'; import type { TaskRunnerState } from './tasks'; import { buildProjectsClosure, filterProjects } from './projectUtils'; import { clearCompilationCache, collectAffectedTestFiles } from '../common/compilationCache'; -import type { FullResult, TestCase } from 'packages/playwright-test/reporter'; +import type { FullResult } from 'packages/playwright-test/reporter'; import chokidar from 'chokidar'; -import { WatchModeReporter } from './reporters'; +import { createReporter, WatchModeReporter } from './reporters'; import { colors } from 'playwright-core/lib/utilsBundle'; import { enquirer } from '../utilsBundle'; import { separator } from '../reporters/base'; @@ -63,11 +63,24 @@ class FSWatcher { } } -export async function runWatchModeLoop(config: FullConfigInternal, failedTests: TestCase[]): Promise { +export async function runWatchModeLoop(config: FullConfigInternal): Promise { + // Perform global setup. + const reporter = await createReporter(config, 'watch'); + const context: TaskRunnerState = { + config, + reporter, + phases: [], + }; + const taskRunner = createTaskRunnerForWatchSetup(config, reporter); + reporter.onConfigure(config); + const { status, cleanup: globalCleanup } = await taskRunner.runDeferCleanup(context, 0); + if (status !== 'passed') + return await globalCleanup(); + const projects = filterProjects(config.projects, config._internal.cliProjectFilter); const projectClosure = buildProjectsClosure(projects); config._internal.passWithNoTests = true; - const failedTestIdCollector = new Set(failedTests.map(t => t.id)); + const failedTestIdCollector = new Set(); const originalCliArgs = config._internal.cliArgs; const originalCliGrep = config._internal.cliGrep; @@ -75,12 +88,14 @@ export async function runWatchModeLoop(config: FullConfigInternal, failedTests: let lastRun: { type: 'changed' | 'regular' | 'failed', failedTestIds?: Set, dirtyFiles?: Set } = { type: 'regular' }; + let result: FullResult['status'] = 'passed'; + const fsWatcher = new FSWatcher(projectClosure.map(p => p.testDir)); while (true) { const sep = separator(); process.stdout.write(` ${sep} -Waiting for file changes. Press ${colors.bold('h')} for help or ${colors.bold('q')} to quit. +Waiting for file changes. Press ${colors.bold('a')} to run all, ${colors.bold('q')} to quit or ${colors.bold('h')} for more options. `); const readCommandPromise = readCommand(); await Promise.race([ @@ -167,11 +182,15 @@ Waiting for file changes. Press ${colors.bold('h')} for help or ${colors.bold('q } if (command === 'exit') - return 'passed'; + break; - if (command === 'interrupted') - return 'interrupted'; + if (command === 'interrupted') { + result = 'interrupted'; + break; + } } + + return result === 'passed' ? await globalCleanup() : result; } async function runChangedTests(config: FullConfigInternal, failedTestIdCollector: Set, projectClosure: FullProjectInternal[], changedFiles: Set) { diff --git a/tests/playwright-test/global-setup.spec.ts b/tests/playwright-test/global-setup.spec.ts index 02d7a0da3d..a283501eb8 100644 --- a/tests/playwright-test/global-setup.spec.ts +++ b/tests/playwright-test/global-setup.spec.ts @@ -366,7 +366,7 @@ test('teardown after error', async ({ runInlineTest }) => { pwt.test('test', () => {}); `, }); - expect(result.exitCode).toBe(0); + expect(result.exitCode).toBe(1); expect(result.passed).toBe(1); const output = result.output; expect(output).toContain('Error: failed teardown 1');