feat(testrunner): allow unexpected passes (#3665)

.github/workflows/tests.yml

@@ -40,8 +40,6 @@ jobs:
     - 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"
       env:
         BROWSER: ${{ matrix.browser }}
-        DEBUG: "pw:*,-pw:wrapped*,-pw:test*"
-        DEBUG_FILE: "test-results/debug.log"
         PWRUNNER_JSON_REPORT: "test-results/report.json"
     - uses: actions/upload-artifact@v1
       if: always()
@@ -67,8 +65,6 @@ jobs:
     - run: node test-runner/cli test/ --jobs=1 --forbid-only --timeout=30000 --retries=3 --reporter=dot,json
       env:
         BROWSER: ${{ matrix.browser }}
-        DEBUG: "pw:*,-pw:wrapped*,-pw:test*"
-        DEBUG_FILE: "test-results/debug.log"
         PWRUNNER_JSON_REPORT: "test-results/report.json"
     - uses: actions/upload-artifact@v1
       if: ${{ always() }}
@@ -98,8 +94,6 @@ jobs:
       shell: bash
       env:
         BROWSER: ${{ matrix.browser }}
-        DEBUG: "pw:*,-pw:wrapped*,-pw:test*"
-        DEBUG_FILE: "test-results/debug.log"
         PWRUNNER_JSON_REPORT: "test-results/report.json"
     - uses: actions/upload-artifact@v1
       if: ${{ always() }}
@@ -152,7 +146,6 @@ jobs:
       env:
         BROWSER: ${{ matrix.browser }}
         HEADLESS: "false"
-        DEBUG_FILE: "test-results/debug.log"
         PWRUNNER_JSON_REPORT: "test-results/report.json"
     - uses: actions/upload-artifact@v1
       if: ${{ always() }}
@@ -184,8 +177,6 @@ jobs:
     - 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"
       env:
         BROWSER: ${{ matrix.browser }}
-        DEBUG: "pw:*,-pw:wrapped*,-pw:test*"
-        DEBUG_FILE: "test-results/debug.log"
         PWWIRE: true
         PWRUNNER_JSON_REPORT: "test-results/report.json"
     - uses: actions/upload-artifact@v1

test-runner/src/builtin.fixtures.ts

@@ -32,6 +32,7 @@ declare global {
 
   type ItFunction<STATE> = ((name: string, inner: (state: STATE) => Promise<void> | void) => void) & {
     fail(condition: boolean): ItFunction<STATE>;
+    fixme(condition: boolean): ItFunction<STATE>;
     flaky(condition: boolean): ItFunction<STATE>;
     skip(condition: boolean): ItFunction<STATE>;
     slow(): ItFunction<STATE>;

test-runner/src/reporters/base.ts

@@ -30,10 +30,9 @@ const stackUtils = new StackUtils();
 
 export class BaseReporter implements Reporter  {
   skipped: Test[] = [];
-  passed: Test[] = [];
+  asExpected: Test[] = [];
+  unexpected = new Set<Test>();
   flaky: Test[] = [];
-  failed: Test[] = [];
-  timedOut: Test[] = [];
   duration = 0;
   startTime: number;
   config: RunnerConfig;
@@ -66,28 +65,24 @@ export class BaseReporter implements Reporter  {
   }
 
   onTestEnd(test: Test, result: TestResult) {
-    switch (result.status) {
-      case 'skipped': {
-        this.skipped.push(test);
-        return;
-      }
-      case 'passed':
-        if (test.results.length === 1)
-          this.passed.push(test);
-        else
-          this.flaky.push(test);
-        return;
-      case 'failed':
-        // Fall through.
-      case 'timedOut': {
-        if (test.results.length === this.config.retries + 1) {
-          if (result.status === 'timedOut')
-            this.timedOut.push(test);
-          else
-            this.failed.push(test);
-        }
-        return;
+    if (result.status === 'skipped') {
+      this.skipped.push(test);
+      return;
+    }
+
+    if (result.status === result.expectedStatus) {
+      if (test.results.length === 1) {
+        // as expected from the first attempt
+        this.asExpected.push(test);
+      } else {
+        // as expected after unexpected -> flaky.
+        this.flaky.push(test);
       }
+      return;
+    }
+    if (result.status === 'passed' || result.status === 'timedOut' || test.results.length === this.config.retries + 1) {
+      // We made as many retries as we could, still failing.
+      this.unexpected.add(test);
     }
   }
 
@@ -98,15 +93,16 @@ export class BaseReporter implements Reporter  {
   epilogue() {
     console.log('');
 
-    console.log(colors.green(`  ${this.passed.length} passed`) + colors.dim(` (${milliseconds(this.duration)})`));
+    console.log(colors.green(`  ${this.asExpected.length} passed`) + colors.dim(` (${milliseconds(this.duration)})`));
 
     if (this.skipped.length)
       console.log(colors.yellow(`  ${this.skipped.length} skipped`));
 
-    if (this.failed.length) {
-      console.log(colors.red(`  ${this.failed.length} failed`));
+    const filteredUnexpected = [...this.unexpected].filter(t => !t._hasResultWithStatus('timedOut'));
+    if (filteredUnexpected.length) {
+      console.log(colors.red(`  ${filteredUnexpected.length} failed`));
       console.log('');
-      this._printFailures(this.failed);
+      this._printFailures(filteredUnexpected);
     }
 
     if (this.flaky.length) {
@@ -115,11 +111,13 @@ export class BaseReporter implements Reporter  {
       this._printFailures(this.flaky);
     }
 
-    if (this.timedOut.length) {
-      console.log(colors.red(`  ${this.timedOut.length} timed out`));
+    const timedOut = [...this.unexpected].filter(t => t._hasResultWithStatus('timedOut'));
+    if (timedOut.length) {
+      console.log(colors.red(`  ${timedOut.length} timed out`));
       console.log('');
-      this._printFailures(this.timedOut);
+      this._printFailures(timedOut);
     }
+    console.log('');
   }
 
   private _printFailures(failures: Test[]) {
@@ -131,7 +129,8 @@ export class BaseReporter implements Reporter  {
   formatFailure(test: Test, index?: number): string {
     const tokens: string[] = [];
     const relativePath = path.relative(process.cwd(), test.file);
-    const header = `  ${index ? index + ')' : ''} ${terminalLink(relativePath, `file://${os.hostname()}${test.file}`)} › ${test.title}`;
+    const passedUnexpectedlySuffix = test.results[0].status === 'passed' ? ' -- passed unexpectedly' : '';
+    const header = `  ${index ? index + ')' : ''} ${terminalLink(relativePath, `file://${os.hostname()}${test.file}`)} › ${test.title}${passedUnexpectedlySuffix}`;
     tokens.push(colors.bold(colors.red(header)));
     for (const result of test.results) {
       if (result.status === 'passed')

test-runner/src/reporters/dot.ts

@@ -23,8 +23,8 @@ class DotReporter extends BaseReporter {
     super.onTestEnd(test, result);
     switch (result.status) {
       case 'skipped': process.stdout.write(colors.yellow('∘')); break;
-      case 'passed': process.stdout.write(colors.green('·')); break;
-      case 'failed': process.stdout.write(colors.red(test.results.length > 1 ? '' + test.results.length : 'F')); break;
+      case 'passed': process.stdout.write(result.status === result.expectedStatus ? colors.green('·') : colors.red('P')); break;
+      case 'failed': process.stdout.write(result.status === result.expectedStatus ? colors.green('f') : colors.red('F')); break;
       case 'timedOut': process.stdout.write(colors.red('T')); break;
     }
   }

test-runner/src/reporters/list.ts

@@ -35,12 +35,14 @@ class ListReporter extends BaseReporter {
   onTestEnd(test: Test, result: TestResult) {
     super.onTestEnd(test, result);
     let text = '';
-    switch (result.status) {
-      case 'skipped': text = colors.green('  - ') + colors.cyan(test.fullTitle()); break;
-      case 'passed': text = '\u001b[2K\u001b[0G' + colors.green('  ✓ ') + colors.gray(test.fullTitle()); break;
-      case 'failed':
-        // fall through
-      case 'timedOut': text = '\u001b[2K\u001b[0G' + colors.red(`  ${++this._failure}) ` + test.fullTitle()); break;
+    if (result.status === 'skipped') {
+      text = colors.green('  - ') + colors.cyan(test.fullTitle());
+    } else {
+      const statusMark = result.status === 'passed' ? colors.green('  ✓ ') : colors.red('  x ');
+      if (result.status === result.expectedStatus)
+        text = '\u001b[2K\u001b[0G' + statusMark + colors.gray(test.fullTitle());
+      else
+        text = '\u001b[2K\u001b[0G' + colors.red(`  ${++this._failure}) ` + test.fullTitle());
     }
     process.stdout.write(text + '\n');
   }

test-runner/src/reporters/pytest.ts

@@ -149,14 +149,15 @@ class PytestReporter extends BaseReporter {
     }
 
     const status = [];
-    if (this.passed.length)
-      status.push(colors.green(`${this.passed.length} passed`));
+    if (this.asExpected.length)
+      status.push(colors.green(`${this.asExpected.length} as expected`));
     if (this.skipped.length)
       status.push(colors.yellow(`${this.skipped.length} skipped`));
-    if (this.failed.length)
-      status.push(colors.red(`${this.failed.length} failed`));
-    if (this.timedOut.length)
-      status.push(colors.red(`${this.timedOut.length} timed out`));
+    const timedOut = [...this.unexpected].filter(t => t._hasResultWithStatus('timedOut'));
+    if (this.unexpected.size - timedOut.length)
+      status.push(colors.red(`${this.unexpected.size - timedOut.length} unexpected failures`));
+    if (timedOut.length)
+      status.push(colors.red(`${timedOut.length} timed out`));
     status.push(colors.dim(`(${milliseconds(Date.now() - this.startTime)})`));
 
     for (let i = lines.length; i < this._visibleRows; ++i)

test-runner/src/runner.ts

@@ -103,7 +103,11 @@ export class Runner {
     let doneCallback;
     const result = new Promise(f => doneCallback = f);
     worker.once('done', params => {
-      if (!params.failedTestId && !params.fatalError) {
+      // We won't file remaining if:
+      // - there are no remaining
+      // - we are here not because something failed
+      // - no unrecoverable worker error
+      if (!params.remaining.length && !params.failedTestId && !params.fatalError) {
         this._workerAvailable(worker);
         doneCallback();
         return;
@@ -112,8 +116,8 @@ export class Runner {
       // When worker encounters error, we will restart it.
       this._restartWorker(worker);
 
-      // In case of fatal error without test id, we are done with the entry.
-      if (params.fatalError && !params.failedTestId) {
+      // In case of fatal error, we are done with the entry.
+      if (params.fatalError) {
         // Report all the tests are failing with this error.
         for (const id of entry.ids) {
           const { test, result } = this._testById.get(id);
@@ -127,13 +131,16 @@ export class Runner {
       }
 
       const remaining = params.remaining;
-      if (this._config.retries) {
+
+      // Only retry expected failures, not passes and only if the test failed.
+      if (this._config.retries && params.failedTestId) {
         const pair = this._testById.get(params.failedTestId);
-        if (pair.test.results.length < this._config.retries + 1) {
+        if (pair.result.expectedStatus === 'passed' && pair.test.results.length < this._config.retries + 1) {
           pair.result = pair.test._appendResult();
           remaining.unshift(pair.test._id);
         }
       }
+
       if (remaining.length)
         this._queue.unshift({ ...entry, ids: remaining });
 
@@ -169,6 +176,7 @@ export class Runner {
       const { test } = this._testById.get(params.id);
       test._skipped = params.skipped;
       test._flaky = params.flaky;
+      test._expectedStatus = params.expectedStatus;
       this._reporter.onTestBegin(test);
     });
     worker.on('testEnd', params => {

test-runner/src/spec.ts

@@ -53,7 +53,7 @@ export function spec(suite: Suite, file: string, timeout: number): () => void {
   const suites = [suite];
   suite.file = file;
 
-  const it = specBuilder(['skip', 'fail', 'slow', 'only', 'flaky'], (specs, title, fn) => {
+  const it = specBuilder(['skip', 'fixme', 'fail', 'slow', 'only', 'flaky'], (specs, title, fn) => {
     const suite = suites[0];
     const test = new Test(title, fn);
     test.file = file;
@@ -65,8 +65,10 @@ export function spec(suite: Suite, file: string, timeout: number): () => void {
       test.only = true;
     if (!only && specs.skip && specs.skip[0])
       test._skipped = true;
-    if (!only && specs.fail && specs.fail[0])
+    if (!only && specs.fixme && specs.fixme[0])
       test._skipped = true;
+    if (specs.fail && specs.fail[0])
+      test._expectedStatus = 'failed';
     if (specs.flaky && specs.flaky[0])
       test._flaky = true;
     suite._addTest(test);
@@ -81,7 +83,9 @@ export function spec(suite: Suite, file: string, timeout: number): () => void {
     if (only)
       child.only = true;
     if (!only && specs.skip && specs.skip[0])
-      child.skipped = true;
+      child._skipped = true;
+    if (!only && specs.fixme && specs.fixme[0])
+      child._skipped = true;
     suites.unshift(child);
     fn();
     suites.shift();

test-runner/src/test.ts

@@ -16,6 +16,8 @@
 
 export type Configuration = { name: string, value: string }[];
 
+type TestStatus = 'passed' | 'failed' | 'timedOut' | 'skipped';
+
 export class Test {
   suite: Suite;
   title: string;
@@ -27,10 +29,13 @@ export class Test {
   results: TestResult[] = [];
 
   _id: string;
+  // Skipped & flaky are resolved based on options in worker only
+  // We will compute them there and send to the runner (front-end)
   _skipped = false;
   _flaky = false;
   _overriddenFn: Function;
   _startTime: number;
+  _expectedStatus: TestStatus = 'passed';
 
   constructor(title: string, fn: Function) {
     this.title = title;
@@ -48,7 +53,7 @@ export class Test {
   _appendResult(): TestResult {
     const result: TestResult = {
       duration: 0,
-      status: 'none',
+      expectedStatus: 'passed',
       stdout: [],
       stderr: [],
       data: {}
@@ -58,17 +63,21 @@ export class Test {
   }
 
   _ok(): boolean {
-    if (this._skipped)
+    if (this._skipped || this.suite._isSkipped())
       return true;
-    const hasFailedResults = !!this.results.find(r => r.status !== 'passed' && r.status !== 'skipped');
+    const hasFailedResults = !!this.results.find(r => r.status !== r.expectedStatus);
     if (!hasFailedResults)
       return true;
     if (!this._flaky)
       return false;
-    const hasPassedResults = !!this.results.find(r => r.status === 'passed');
+    const hasPassedResults = !!this.results.find(r => r.status === r.expectedStatus);
     return hasPassedResults;
   }
 
+  _hasResultWithStatus(status: TestStatus): boolean {
+    return !!this.results.find(r => r.status === status);
+  }
+
   _clone(): Test {
     const test = new Test(this.title, this.fn);
     test.suite = this.suite;
@@ -83,7 +92,8 @@ export class Test {
 
 export type TestResult = {
   duration: number;
-  status: 'none' | 'passed' | 'failed' | 'timedOut' | 'skipped';
+  status?: TestStatus;
+  expectedStatus: TestStatus;
   error?: any;
   stdout: (string | Buffer)[];
   stderr: (string | Buffer)[];
@@ -96,9 +106,12 @@ export class Suite {
   suites: Suite[] = [];
   tests: Test[] = [];
   only = false;
-  skipped = false;
   file: string;
   configuration: Configuration;
+
+  // Skipped & flaky are resolved based on options in worker only
+  // We will compute them there and send to the runner (front-end)
+  _skipped = false;
   _configurationString: string;
 
   _hooks: { type: string, fn: Function } [] = [];
@@ -124,7 +137,7 @@ export class Suite {
   }
 
   _isSkipped(): boolean {
-    return this.skipped || (this.parent && this.parent._isSkipped());
+    return this._skipped || (this.parent && this.parent._isSkipped());
   }
 
   _addTest(test: Test) {
@@ -163,7 +176,7 @@ export class Suite {
     const suite = new Suite(this.title);
     suite.only = this.only;
     suite.file = this.file;
-    suite.skipped = this.skipped;
+    suite._skipped = this._skipped;
     return suite;
   }