fix(testrunner): include fixture teardown into timeout, add global ti…

.github/workflows/auto_roll.yml

@@ -27,7 +27,7 @@ jobs:
     # XVFB-RUN merges both STDOUT and STDERR, whereas we need only STDERR
     # Wrap `npm run` in a subshell to redirect STDERR to file.
     # Enable core dumps in the subshell.
-    - run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- bash -c "ulimit -c unlimited && node test-runner/cli test/ --jobs=1 --forbid-only --retries=3 --timeout=30000 --reporter=dot,json"
+    - run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- bash -c "ulimit -c unlimited && node test-runner/cli test/ --jobs=1 --forbid-only --retries=3 --timeout=30000 --global-timeout=5400000 --reporter=dot,json"
       env:
         BROWSER: ${{ matrix.browser }}
         DEBUG: "pw:*,-pw:wrapped*,-pw:test*"

.github/workflows/tests.yml

@@ -37,7 +37,7 @@ jobs:
     # XVFB-RUN merges both STDOUT and STDERR, whereas we need only STDERR
     # Wrap `npm run` in a subshell to redirect STDERR to file.
     # Enable core dumps in the subshell.
-    - run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- bash -c "ulimit -c unlimited && node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --retries=3 --reporter=dot,json && npm run coverage"
+    - run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- bash -c "ulimit -c unlimited && node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --global-timeout=5400000 --retries=3 --reporter=dot,json && npm run coverage"
       env:
         BROWSER: ${{ matrix.browser }}
         PWRUNNER_JSON_REPORT: "test-results/report.json"
@@ -62,7 +62,7 @@ jobs:
     - uses: microsoft/playwright-github-action@v1
     - run: npm ci
     - run: npm run build
-    - run: node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --retries=3 --reporter=dot,json
+    - run: node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --global-timeout=5400000 --retries=3 --reporter=dot,json
       env:
         BROWSER: ${{ matrix.browser }}
         PWRUNNER_JSON_REPORT: "test-results/report.json"
@@ -90,7 +90,7 @@ jobs:
     - uses: microsoft/playwright-github-action@v1
     - run: npm ci
     - run: npm run build
-    - run: node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --retries=3 --reporter=dot,json
+    - run: node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --global-timeout=5400000 --retries=3 --reporter=dot,json
       shell: bash
       env:
         BROWSER: ${{ matrix.browser }}
@@ -141,7 +141,7 @@ jobs:
     # XVFB-RUN merges both STDOUT and STDERR, whereas we need only STDERR
     # Wrap `npm run` in a subshell to redirect STDERR to file.
     # Enable core dumps in the subshell.
-    - run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- bash -c "ulimit -c unlimited && node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --retries=3 --reporter=dot,json"
+    - run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- bash -c "ulimit -c unlimited && node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --global-timeout=5400000 --retries=3 --reporter=dot,json"
       if: ${{ always() }}
       env:
         BROWSER: ${{ matrix.browser }}
@@ -174,7 +174,7 @@ jobs:
     # XVFB-RUN merges both STDOUT and STDERR, whereas we need only STDERR
     # Wrap `npm run` in a subshell to redirect STDERR to file.
     # Enable core dumps in the subshell.
-    - run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- bash -c "ulimit -c unlimited && node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --retries=3 --reporter=dot,json"
+    - run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- bash -c "ulimit -c unlimited && node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --global-timeout=5400000 --retries=3 --reporter=dot,json"
       env:
         BROWSER: ${{ matrix.browser }}
         PWWIRE: true
@@ -206,7 +206,7 @@ jobs:
     # XVFB-RUN merges both STDOUT and STDERR, whereas we need only STDERR
     # Wrap `npm run` in a subshell to redirect STDERR to file.
     # Enable core dumps in the subshell.
-    - run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- bash -c "ulimit -c unlimited && node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --retries=3 --reporter=dot,json"
+    - run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- bash -c "ulimit -c unlimited && node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --global-timeout=5400000 --retries=3 --reporter=dot,json"
       env:
         BROWSER: ${{ matrix.browser }}
         TRACING: true

test-runner/src/cli.ts

@@ -36,6 +36,7 @@ program
     .version('Version ' + /** @type {any} */ (require)('../package.json').version)
     .option('--forbid-only', 'Fail if exclusive test(s) encountered', false)
     .option('-g, --grep <grep>', 'Only run tests matching this string or regexp', '.*')
+    .option('--global-timeout <timeout>', 'Specify maximum time this test suite can run (in milliseconds), default: 0 for unlimited', '0')
     .option('-j, --jobs <jobs>', 'Number of concurrent jobs for --parallel; use 1 to run in serial, default: (number of CPU cores / 2)', String(Math.ceil(require('os').cpus().length / 2)))
     .option('--reporter <reporter>', 'Specify reporter to use, comma-separated, can be "dot", "list", "json"', 'dot')
     .option('--repeat-each <repeat-each>', 'Specify how many times to run the tests', '1')
@@ -60,6 +61,7 @@ program
         snapshotDir: path.join(testDir, '__snapshots__'),
         testDir,
         timeout: parseInt(command.timeout, 10),
+        globalTimeout: parseInt(command.globalTimeout, 10),
         trialRun: command.trialRun,
         updateSnapshots: command.updateSnapshots
       };

test-runner/src/fixtures.ts

@@ -17,6 +17,7 @@
 import debug from 'debug';
 import { RunnerConfig } from './runnerConfig';
 import { serializeError, Test, TestResult } from './test';
+import { raceAgainstTimeout } from './util';
 
 type Scope = 'test' | 'worker';
 
@@ -152,24 +153,32 @@ export class FixturePool {
     return fn(params);
   }
 
-  async runTestWithFixtures(fn: Function, timeout: number, info: TestInfo) {
-    let timer: NodeJS.Timer;
-    const timerPromise = new Promise(f => timer = setTimeout(f, timeout));
+  async runTestWithFixturesAndTimeout(fn: Function, timeout: number, info: TestInfo) {
+    const { timedOut } = await raceAgainstTimeout(this._runTestWithFixtures(fn, info), timeout);
+    // Do not overwrite test failure upon timeout in fixture.
+    if (timedOut && info.result.status === 'passed')
+      info.result.status = 'timedOut';
+  }
+
+  async _runTestWithFixtures(fn: Function, info: TestInfo) {
+    try {
+      await this.resolveParametersAndRun(fn, info.config, info);
+      info.result.status = 'passed';
+    } catch (error) {
+      // Prefer original error to the fixture teardown error or timeout.
+      if (info.result.status === 'passed') {
+        info.result.status = 'failed';
+        info.result.error = serializeError(error);
+      }
+    }
     try {
-      await Promise.race([
-        this.resolveParametersAndRun(fn, info.config, info).then(() => {
-          info.result.status = 'passed';
-          clearTimeout(timer);
-        }).catch(e => {
-          info.result.status = 'failed';
-          info.result.error = serializeError(e);
-        }),
-        timerPromise.then(() => {
-          info.result.status = 'timedOut';
-        })
-      ]);
-    } finally {
       await this.teardownScope('test');
+    } catch (error) {
+      // Prefer original error to the fixture teardown error or timeout.
+      if (info.result.status === 'passed') {
+        info.result.status = 'failed';
+        info.result.error = serializeError(error);
+      }
     }
   }
 }

test-runner/src/index.ts

@@ -25,8 +25,10 @@ import { registerFixture as registerFixtureT, registerWorkerFixture as registerW
 import { Reporter } from './reporter';
 import { Runner } from './runner';
 import { RunnerConfig } from './runnerConfig';
+import { Suite } from './test';
 import { Matrix, TestCollector } from './testCollector';
 import { installTransform } from './transform';
+import { raceAgainstTimeout } from './util';
 export { parameters, registerParameter } from './fixtures';
 export { Reporter } from './reporter';
 export { RunnerConfig } from './runnerConfig';
@@ -93,7 +95,15 @@ export async function run(config: RunnerConfig, files: string[], reporter: Repor
   const total = suite.total();
   if (!total)
     return 'no-tests';
+  const { result, timedOut } = await raceAgainstTimeout(runTests(config, suite, reporter), config.globalTimeout);
+  if (timedOut) {
+    reporter.onTimeout(config.globalTimeout);
+    process.exit(1);
+  }
+  return result;
+}
 
+async function runTests(config: RunnerConfig, suite: Suite, reporter: Reporter) {
   // Trial run does not need many workers, use one.
   const jobs = (config.trialRun || config.debug) ? 1 : config.jobs;
   const runner = new Runner(suite, { ...config, jobs }, reporter);

test-runner/src/reporter.ts

@@ -20,8 +20,9 @@ import { Suite, Test, TestResult } from './test';
 export interface Reporter {
   onBegin(config: RunnerConfig, suite: Suite): void;
   onTestBegin(test: Test): void;
-  onTestStdOut(test: Test, chunk: string | Buffer);
-  onTestStdErr(test: Test, chunk: string | Buffer);
-  onTestEnd(test: Test, result: TestResult);
+  onTestStdOut(test: Test, chunk: string | Buffer): void;
+  onTestStdErr(test: Test, chunk: string | Buffer): void;
+  onTestEnd(test: Test, result: TestResult): void;
+  onTimeout(timeout: number): void;
   onEnd(): void;
 }

test-runner/src/reporters/base.ts

@@ -32,11 +32,13 @@ export class BaseReporter implements Reporter  {
   skipped: Test[] = [];
   asExpected: Test[] = [];
   unexpected = new Set<Test>();
-  flaky: Test[] = [];
+  expectedFlaky: Test[] = [];
+  unexpectedFlaky: Test[] = [];
   duration = 0;
   startTime: number;
   config: RunnerConfig;
   suite: Suite;
+  timeout: number;
 
   constructor() {
     process.on('SIGINT', async () => {
@@ -76,7 +78,10 @@ export class BaseReporter implements Reporter  {
         this.asExpected.push(test);
       } else {
         // as expected after unexpected -> flaky.
-        this.flaky.push(test);
+        if (test.isFlaky())
+          this.expectedFlaky.push(test);
+        else
+          this.unexpectedFlaky.push(test);
       }
       return;
     }
@@ -86,6 +91,10 @@ export class BaseReporter implements Reporter  {
     }
   }
 
+  onTimeout(timeout: number) {
+    this.timeout = timeout;
+  }
+
   onEnd() {
     this.duration = Date.now() - this.startTime;
   }
@@ -105,10 +114,13 @@ export class BaseReporter implements Reporter  {
       this._printFailures(filteredUnexpected);
     }
 
-    if (this.flaky.length) {
-      console.log(colors.red(`  ${this.flaky.length} flaky`));
-      console.log('');
-      this._printFailures(this.flaky);
+    const allFlaky = this.expectedFlaky.length + this.unexpectedFlaky.length;
+    if (allFlaky) {
+      console.log(colors.red(`  ${allFlaky} flaky`));
+      if (this.unexpectedFlaky.length) {
+        console.log('');
+        this._printFailures(this.unexpectedFlaky);
+      }
     }
 
     const timedOut = [...this.unexpected].filter(t => t._hasResultWithStatus('timedOut'));
@@ -118,6 +130,10 @@ export class BaseReporter implements Reporter  {
       this._printFailures(timedOut);
     }
     console.log('');
+    if (this.timeout) {
+      console.log(colors.red(`  Timed out waiting ${this.timeout / 1000}s for the entire test run`));
+      console.log('');
+    }
   }
 
   private _printFailures(failures: Test[]) {

test-runner/src/reporters/dot.ts

@@ -29,6 +29,11 @@ class DotReporter extends BaseReporter {
     }
   }
 
+  onTimeout(timeout) {
+    super.onTimeout(timeout);
+    this.onEnd();
+  }
+
   onEnd() {
     super.onEnd();
     process.stdout.write('\n');

test-runner/src/reporters/json.ts

@@ -19,6 +19,11 @@ import { Suite, Test, TestResult } from '../test';
 import * as fs from 'fs';
 
 class JSONReporter extends BaseReporter {
+  onTimeout(timeout) {
+    super.onTimeout(timeout);
+    this.onEnd();
+  }
+
   onEnd() {
     super.onEnd();
     const result = {

test-runner/src/reporters/multiplexer.ts

@@ -50,6 +50,11 @@ export class Multiplexer implements Reporter {
       reporter.onTestEnd(test, result);
   }
 
+  onTimeout(timeout: number) {
+    for (const reporter of this._reporters)
+      reporter.onTimeout(timeout);
+  }
+
   onEnd() {
     for (const reporter of this._reporters)
       reporter.onEnd();

test-runner/src/runnerConfig.ts

@@ -15,17 +15,18 @@
  */
 
 export type RunnerConfig = {
+  debug?: boolean;
   forbidOnly?: boolean;
+  globalTimeout: number;
+  grep?: string;
   jobs: number;
   outputDir: string;
-  snapshotDir: string;
-  testDir: string;
-  timeout: number;
-  debug?: boolean;
   quiet?: boolean;
-  grep?: string;
   repeatEach: number;
   retries: number,
+  snapshotDir: string;
+  testDir: string;
+  timeout: number;
   trialRun?: boolean;
   updateSnapshots?: boolean;
 };

test-runner/src/test.ts

@@ -103,8 +103,8 @@ export class Runnable {
     return this._slow || (this.parent && this.parent._isSlow());
   }
 
-  _isFlaky(): boolean {
-    return this._flaky || (this.parent && this.parent._isFlaky());
+  isFlaky(): boolean {
+    return this._flaky || (this.parent && this.parent.isFlaky());
   }
 
   titlePath(): string[] {
@@ -162,7 +162,7 @@ export class Test extends Runnable {
     const hasFailedResults = !!this.results.find(r => r.status !== r.expectedStatus);
     if (!hasFailedResults)
       return true;
-    if (!this._isFlaky())
+    if (!this.isFlaky())
       return false;
     const hasPassedResults = !!this.results.find(r => r.status === r.expectedStatus);
     return hasPassedResults;

test-runner/src/testRunner.ts

@@ -158,7 +158,7 @@ export class TestRunner extends EventEmitter {
     // We only know resolved skipped/flaky value in the worker,
     // send it to the runner.
     test._skipped = test._isSkipped();
-    test._flaky = test._isFlaky();
+    test._flaky = test.isFlaky();
     test._slow = test._isSlow();
     this.emit('testBegin', {
       id,
@@ -189,7 +189,7 @@ export class TestRunner extends EventEmitter {
       if (!this._trialRun) {
         await this._runHooks(test.parent, 'beforeEach', 'before', testInfo);
         const timeout = test._isSlow() ? this._timeout * 3 : this._timeout;
-        await fixturePool.runTestWithFixtures(test.fn, timeout, testInfo);
+        await fixturePool.runTestWithFixturesAndTimeout(test.fn, timeout, testInfo);
         await this._runHooks(test.parent, 'afterEach', 'after', testInfo);
       } else {
         result.status = result.expectedStatus;

test-runner/src/util.ts

@@ -0,0 +1,45 @@
+/**
+ * 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.
+ */
+
+export async function raceAgainstTimeout<T>(promise: Promise<T>, timeout: number): Promise<{ result?: T, timedOut?: boolean }> {
+  if (!timeout)
+    return { result: await promise };
+
+  let timer: NodeJS.Timer;
+  let done = false;
+  let fulfill: (t: { result?: T, timedOut?: boolean }) => void;
+  let reject: (e: Error) => void;
+  const result = new Promise((f, r) => {
+    fulfill = f;
+    reject = r;
+  });
+  setTimeout(() => {
+    done = true;
+    fulfill({ timedOut: true });
+  }, timeout);
+  promise.then(result => {
+    clearTimeout(timer);
+    if (!done) {
+      done = true;
+      fulfill({ result });
+    }
+  }).catch(e => {
+    clearTimeout(timer);
+    if (!done)
+      reject(e);
+  });
+  return result;
+}