From e89de7e66a3d988633f99b83699a13922a7622cf Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 24 Aug 2020 18:58:43 -0700 Subject: [PATCH] fix(testrunner): allow worker fixture to throw/timeout (#3610) --- test-runner/src/fixtures.ts | 22 ++++++------- test-runner/src/runner.ts | 1 + test-runner/src/test.ts | 2 +- test-runner/src/testRunner.ts | 32 +++++++++++++------ test-runner/src/worker.ts | 12 ++++++- .../test/assets/worker-fixture-error.js | 24 ++++++++++++++ .../test/assets/worker-fixture-timeout.js | 23 +++++++++++++ test-runner/test/exit-code.spec.ts | 19 +++++++++-- 8 files changed, 108 insertions(+), 27 deletions(-) create mode 100644 test-runner/test/assets/worker-fixture-error.js create mode 100644 test-runner/test/assets/worker-fixture-timeout.js diff --git a/test-runner/src/fixtures.ts b/test-runner/src/fixtures.ts index fb4e044de5..ea5f1c8b33 100644 --- a/test-runner/src/fixtures.ts +++ b/test-runner/src/fixtures.ts @@ -101,8 +101,8 @@ class Fixture { if (this._setup) { debug('pw:test:hook')(`teardown "${this.name}"`); this._teardownFenceCallback(); + await this._tearDownComplete; } - await this._tearDownComplete; this.pool.instances.delete(this.name); } } @@ -134,31 +134,27 @@ export class FixturePool { } } - async resolveParametersAndRun(fn: Function, timeout: number, config: Config, test?: Test) { + async resolveParametersAndRun(fn: Function, config: Config, test?: Test) { const names = fixtureParameterNames(fn); for (const name of names) await this.setupFixture(name, config, test); const params = {}; for (const n of names) params[n] = this.instances.get(n).value; - - if (!timeout) - return fn(params); - - let timer: NodeJS.Timer; - let timerPromise = new Promise(f => timer = setTimeout(f, timeout)); - return Promise.race([ - Promise.resolve(fn(params)).then(() => clearTimeout(timer)), - timerPromise.then(() => Promise.reject(new Error(`Timeout of ${timeout}ms exceeded`))) - ]); + return fn(params); } wrapTestCallback(callback: any, timeout: number, config: Config, test: Test) { if (!callback) return callback; return async() => { + let timer: NodeJS.Timer; + let timerPromise = new Promise(f => timer = setTimeout(f, timeout)); try { - await this.resolveParametersAndRun(callback, timeout, config, test); + await Promise.race([ + this.resolveParametersAndRun(callback, config, test).then(() => clearTimeout(timer)), + timerPromise.then(() => Promise.reject(new Error(`Timeout of ${timeout}ms exceeded`))) + ]); } catch (e) { test.error = serializeError(e); throw e; diff --git a/test-runner/src/runner.ts b/test-runner/src/runner.ts index 48d56859a6..560e5339e9 100644 --- a/test-runner/src/runner.ts +++ b/test-runner/src/runner.ts @@ -233,6 +233,7 @@ class OopWorker extends Worker { stdio: ['ignore', 'ignore', 'ignore', 'ipc'] }); this.process.on('exit', () => this.emit('exit')); + this.process.on('error', (e) => {}); // do not yell at a send to dead process. this.process.on('message', message => { const { method, params } = message; this.emit(method, params); diff --git a/test-runner/src/test.ts b/test-runner/src/test.ts index cc3ee9293d..81f683d5bb 100644 --- a/test-runner/src/test.ts +++ b/test-runner/src/test.ts @@ -165,7 +165,7 @@ export function serializeConfiguration(configuration: Configuration): string { return tokens.join(', '); } -export function serializeError(error: Error): any { +export function serializeError(error: Error | any): any { if (error instanceof Error) { return { message: error.message, diff --git a/test-runner/src/testRunner.ts b/test-runner/src/testRunner.ts index 722f89ed24..72bf55f1b4 100644 --- a/test-runner/src/testRunner.ts +++ b/test-runner/src/testRunner.ts @@ -43,6 +43,7 @@ export class TestRunner extends EventEmitter { private _parsedGeneratorConfiguration: any = {}; private _config: RunnerConfig; private _timeout: number; + private _test: Test | null = null; constructor(entry: TestRunnerEntry, config: RunnerConfig, workerId: number) { super(); @@ -63,6 +64,17 @@ export class TestRunner extends EventEmitter { this._trialRun = true; } + fatalError(error: Error | any) { + this._fatalError = serializeError(error); + if (this._test) { + this._test.error = this._fatalError; + this.emit('fail', { + test: this._serializeTest(), + }); + } + this._reportDone(); + } + async run() { setParameters(this._parsedGeneratorConfiguration); @@ -102,30 +114,32 @@ export class TestRunner extends EventEmitter { private async _runTest(test: Test) { if (this._failedWithError) return false; + this._test = test; const ordinal = ++this._currentOrdinal; if (this._ordinals.size && !this._ordinals.has(ordinal)) return; this._remaining.delete(ordinal); if (test.pending || test.suite._isPending()) { - this.emit('pending', { test: this._serializeTest(test) }); + this.emit('pending', { test: this._serializeTest() }); return; } - this.emit('test', { test: this._serializeTest(test) }); + this.emit('test', { test: this._serializeTest() }); try { await this._runHooks(test.suite, 'beforeEach', 'before'); test._startTime = Date.now(); if (!this._trialRun) await this._testWrapper(test)(); - this.emit('pass', { test: this._serializeTest(test) }); + this.emit('pass', { test: this._serializeTest() }); await this._runHooks(test.suite, 'afterEach', 'after'); } catch (error) { test.error = serializeError(error); this._failedWithError = test.error; this.emit('fail', { - test: this._serializeTest(test), + test: this._serializeTest(), }); } + this._test = null; } private async _runHooks(suite: Suite, type: string, dir: 'before' | 'after') { @@ -139,7 +153,7 @@ export class TestRunner extends EventEmitter { if (dir === 'before') all.reverse(); for (const hook of all) - await fixturePool.resolveParametersAndRun(hook, 0, this._config); + await fixturePool.resolveParametersAndRun(hook, this._config); } private _reportDone() { @@ -155,11 +169,11 @@ export class TestRunner extends EventEmitter { return fixturePool.wrapTestCallback(test.fn, timeout, { ...this._config }, test); } - private _serializeTest(test) { + private _serializeTest() { return { - id: `${test._ordinal}@${this._configuredFile}`, - error: test.error, - duration: Date.now() - test._startTime, + id: `${this._test._ordinal}@${this._configuredFile}`, + error: this._test.error, + duration: Date.now() - this._test._startTime, }; } } diff --git a/test-runner/src/worker.ts b/test-runner/src/worker.ts index a1573e6228..4c0c171157 100644 --- a/test-runner/src/worker.ts +++ b/test-runner/src/worker.ts @@ -47,6 +47,16 @@ process.on('SIGTERM',() => {}); let workerId: number; let testRunner: TestRunner; +process.on('unhandledRejection', (reason, promise) => { + if (testRunner && !closed) + testRunner.fatalError(reason); +}); + +process.on('uncaughtException', error => { + if (testRunner && !closed) + testRunner.fatalError(error); +}); + process.on('message', async message => { if (message.method === 'init') { workerId = message.params.workerId; @@ -76,7 +86,7 @@ async function gracefullyCloseAndExit() { setTimeout(() => process.exit(0), 30000); // Meanwhile, try to gracefully close all browsers. if (testRunner) - await testRunner.stop(); + testRunner.stop(); await fixturePool.teardownScope('worker'); process.exit(0); } diff --git a/test-runner/test/assets/worker-fixture-error.js b/test-runner/test/assets/worker-fixture-error.js new file mode 100644 index 0000000000..d9130b6ee6 --- /dev/null +++ b/test-runner/test/assets/worker-fixture-error.js @@ -0,0 +1,24 @@ +/** + * Copyright (c) Microsoft Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const { registerWorkerFixture } = require('../../'); + +registerWorkerFixture('failure', async ({}, runTest) => { + throw new Error('Worker failed'); +}); + +it('fails', async({failure}) => { +}); diff --git a/test-runner/test/assets/worker-fixture-timeout.js b/test-runner/test/assets/worker-fixture-timeout.js new file mode 100644 index 0000000000..2aabcae9fb --- /dev/null +++ b/test-runner/test/assets/worker-fixture-timeout.js @@ -0,0 +1,23 @@ +/** + * Copyright (c) Microsoft Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const { registerWorkerFixture } = require('../../'); + +registerWorkerFixture('timeout', async ({}, runTest) => { +}); + +it('fails', async({timeout}) => { +}); diff --git a/test-runner/test/exit-code.spec.ts b/test-runner/test/exit-code.spec.ts index 92b84f46c1..e2e5a091fb 100644 --- a/test-runner/test/exit-code.spec.ts +++ b/test-runner/test/exit-code.spec.ts @@ -44,20 +44,33 @@ it('should access error in fixture', async() => { expect(data.message).toContain('Object.is equality'); }); -async function runTest(filePath: string) { +it('should handle worker fixture timeout', async() => { + const result = await runTest('worker-fixture-timeout.js', 1000); + expect(result.exitCode).toBe(1); + expect(result.output).toContain('Timeout of 1000ms'); +}); + +it('should handle worker fixture error', async() => { + const result = await runTest('worker-fixture-error.js'); + expect(result.exitCode).toBe(1); + expect(result.output).toContain('Worker failed'); +}); + +async function runTest(filePath: string, timeout = 10000) { const outputDir = path.join(__dirname, 'test-results') await removeFolderAsync(outputDir).catch(e => {}); const { output, status } = spawnSync('node', [ path.join(__dirname, '..', 'cli.js'), path.join(__dirname, 'assets', filePath), - '--output=' + outputDir + '--output=' + outputDir, + '--timeout=' + timeout ]); const passed = (/(\d+) passed/.exec(output.toString()) || [])[1]; const failed = (/(\d+) failed/.exec(output.toString()) || [])[1]; return { exitCode: status, - output, + output: output.toString(), passed: parseInt(passed), failed: parseInt(failed || '0') }