fix(test runner): do not special case test.fail (#8447)

This makes `test.fail` tests considered as passing when they actually fail:
- Stop restarting the worker.
- Retry when it passes instead of a fail.
- Behaves similar to regular tests in a `describe.serial` suite.

src/test/dispatcher.ts

@@ -192,10 +192,9 @@ export class Dispatcher {
         }
       }
 
-      // Only retry expected failures, not passes and only if the test failed.
       for (const testId of retryCandidates) {
         const pair = this._testById.get(testId)!;
-        if (!this._isStopped && pair.test.expectedStatus === 'passed' && pair.test.results.length < pair.test.retries + 1) {
+        if (!this._isStopped && pair.test.results.length < pair.test.retries + 1) {
           pair.result = pair.test._appendTestResult();
           pair.steps = new Map();
           pair.stepStack = new Set();

src/test/reporters/base.ts

@@ -147,8 +147,8 @@ export class BaseReporter implements Reporter  {
     });
   }
 
-  willRetry(test: TestCase, result: TestResult): boolean {
+  willRetry(test: TestCase): boolean {
-    return test.outcome() === 'unexpected' && result.status !== 'passed' && test.results.length <= test.retries;
+    return test.outcome() === 'unexpected' && test.results.length <= test.retries;
   }
 }
 
@@ -159,10 +159,9 @@ export function formatFailure(config: FullConfig, test: TestCase, index?: number
     const resultTokens = formatResultFailure(test, result, '    ');
     if (!resultTokens.length)
       continue;
-    const statusSuffix = (result.status === 'passed' && test.expectedStatus === 'failed') ? ' -- passed unexpectedly' : '';
     if (result.retry) {
       tokens.push('');
-      tokens.push(colors.gray(pad(`    Retry #${result.retry}${statusSuffix}`, '-')));
+      tokens.push(colors.gray(pad(`    Retry #${result.retry}`, '-')));
     }
     tokens.push(...resultTokens);
     const output = ((result as any)[kOutputSymbol] || []) as TestResultOutput[];
@@ -187,6 +186,10 @@ export function formatResultFailure(test: TestCase, result: TestResult, initialI
     resultTokens.push('');
     resultTokens.push(indent(colors.red(`Timeout of ${test.timeout}ms exceeded.`), initialIndent));
   }
+  if (result.status === 'passed' && test.expectedStatus === 'failed') {
+    resultTokens.push('');
+    resultTokens.push(indent(colors.red(`Expected to fail, but passed.`), initialIndent));
+  }
   if (result.error !== undefined)
     resultTokens.push(indent(formatError(result.error, test.location.file), initialIndent));
   return resultTokens;
@@ -211,8 +214,7 @@ export function formatTestTitle(config: FullConfig, test: TestCase, step?: TestS
 
 function formatTestHeader(config: FullConfig, test: TestCase, indent: string, index?: number): string {
   const title = formatTestTitle(config, test);
-  const passedUnexpectedlySuffix = (test.results[0].status === 'passed' && test.expectedStatus === 'failed') ? ' -- passed unexpectedly' : '';
+  const header = `${indent}${index ? index + ') ' : ''}${title}`;
-  const header = `${indent}${index ? index + ') ' : ''}${title}${passedUnexpectedlySuffix}`;
   return colors.red(pad(header, '='));
 }
 

src/test/reporters/dot.ts

@@ -44,7 +44,7 @@ class DotReporter extends BaseReporter {
       process.stdout.write(colors.yellow('°'));
       return;
     }
-    if (this.willRetry(test, result)) {
+    if (this.willRetry(test)) {
       process.stdout.write(colors.gray('×'));
       return;
     }

src/test/reporters/line.ts

@@ -59,7 +59,7 @@ class LineReporter extends BaseReporter {
     const width = process.stdout.columns! - 1;
     const title = `[${++this._current}/${this._total}] ${formatTestTitle(this.config, test)}`.substring(0, width);
     process.stdout.write(`\u001B[1A\u001B[2K${title}\n`);
-    if (!this.willRetry(test, result) && (test.outcome() === 'flaky' || test.outcome() === 'unexpected')) {
+    if (!this.willRetry(test) && (test.outcome() === 'flaky' || test.outcome() === 'unexpected')) {
       process.stdout.write(`\u001B[1A\u001B[2K`);
       console.log(formatFailure(this.config, test, ++this._failures));
       console.log();

src/test/workerRunner.ts

@@ -365,7 +365,7 @@ export class WorkerRunner extends EventEmitter {
     if (reportEvents)
       this.emit('testEnd', buildTestEndPayload(testId, testInfo));
 
-    const isFailure = testInfo.status === 'timedOut' || (testInfo.status === 'failed' && testInfo.expectedStatus !== 'failed');
+    const isFailure = testInfo.status !== 'skipped' && testInfo.status !== testInfo.expectedStatus;
     const preserveOutput = this._loader.fullConfig().preserveOutput === 'always' ||
       (this._loader.fullConfig().preserveOutput === 'failures-only' && isFailure);
     if (!preserveOutput)
@@ -374,7 +374,7 @@ export class WorkerRunner extends EventEmitter {
     this._currentTest = null;
     setCurrentTestInfo(null);
 
-    if (testInfo.status !== 'passed' && testInfo.status !== 'skipped') {
+    if (isFailure) {
       if (test._type === 'test')
         this._failedTestId = testId;
       else if (!this._fatalError)

tests/playwright-test/exit-code.spec.ts

@@ -116,7 +116,7 @@ test('should fail on unexpected pass', async ({ runInlineTest }) => {
   });
   expect(exitCode).toBe(1);
   expect(failed).toBe(1);
-  expect(output).toContain('passed unexpectedly');
+  expect(output).toContain('Expected to fail, but passed');
 });
 
 test('should respect global timeout', async ({ runInlineTest }) => {

tests/playwright-test/retry.spec.ts

@@ -87,10 +87,10 @@ test('should fail on unexpected pass with retries', async ({ runInlineTest }) =>
   }, { retries: 1 });
   expect(exitCode).toBe(1);
   expect(failed).toBe(1);
-  expect(output).toContain('passed unexpectedly');
+  expect(output).toContain('Expected to fail, but passed.');
 });
 
-test('should not retry unexpected pass', async ({ runInlineTest }) => {
+test('should retry unexpected pass', async ({ runInlineTest }) => {
   const { exitCode, passed, failed, output } = await runInlineTest({
     'unexpected-pass.spec.js': `
       const { test } = pwt;
@@ -103,7 +103,7 @@ test('should not retry unexpected pass', async ({ runInlineTest }) => {
   expect(exitCode).toBe(1);
   expect(passed).toBe(0);
   expect(failed).toBe(1);
-  expect(stripAscii(output).split('\n')[0]).toBe('F');
+  expect(stripAscii(output).split('\n')[0]).toBe('××F');
 });
 
 test('should not retry expected failure', async ({ runInlineTest }) => {

tests/playwright-test/test-serial.spec.ts

@@ -129,3 +129,84 @@ test('test.describe.serial.only should work', async ({ runInlineTest }) => {
     '%%test3',
   ]);
 });
+
+test('test.describe.serial should work with test.fail', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      const { test } = pwt;
+      test.describe.serial('suite', () => {
+        test('zero', () => {
+          console.log('\\n%%zero');
+        });
+
+        test('one', ({}) => {
+          console.log('\\n%%one');
+          test.fail();
+          expect(1).toBe(2);
+        });
+
+        test('two', ({}, testInfo) => {
+          console.log('\\n%%two');
+          test.fail();
+          expect(testInfo.retry).toBe(0);
+        });
+
+        test('three', () => {
+          console.log('\\n%%three');
+        });
+      });
+    `,
+  }, { retries: 0 });
+  expect(result.exitCode).toBe(1);
+  expect(result.passed).toBe(2);
+  expect(result.failed).toBe(1);
+  expect(result.skipped).toBe(1);
+  expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
+    '%%zero',
+    '%%one',
+    '%%two',
+  ]);
+});
+
+test('test.describe.serial should work with test.fail and retries', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      const { test } = pwt;
+      test.describe.serial('suite', () => {
+        test('zero', () => {
+          console.log('\\n%%zero');
+        });
+
+        test('one', ({}) => {
+          console.log('\\n%%one');
+          test.fail();
+          expect(1).toBe(2);
+        });
+
+        test('two', ({}, testInfo) => {
+          console.log('\\n%%two');
+          test.fail();
+          expect(testInfo.retry).toBe(0);
+        });
+
+        test('three', () => {
+          console.log('\\n%%three');
+        });
+      });
+    `,
+  }, { retries: 1 });
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(3);
+  expect(result.flaky).toBe(1);
+  expect(result.failed).toBe(0);
+  expect(result.skipped).toBe(0);
+  expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
+    '%%zero',
+    '%%one',
+    '%%two',
+    '%%zero',
+    '%%one',
+    '%%two',
+    '%%three',
+  ]);
+});

tests/playwright-test/worker-index.spec.ts

@@ -115,7 +115,7 @@ test('should reuse worker after test.skip()', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(0);
 });
 
-test('should use new worker after test.fail()', async ({ runInlineTest }) => {
+test('should not use new worker after test.fail()', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.test.js': `
       const { test } = pwt;
@@ -129,7 +129,7 @@ test('should use new worker after test.fail()', async ({ runInlineTest }) => {
       });
 
       test('succeeds 2', async ({}, testInfo) => {
-        expect(testInfo.workerIndex).toBe(1);
+        expect(testInfo.workerIndex).toBe(0);
       });
     `,
   });