fix(reporters): properly determine flaky status for serial mode (#30529)

There are plenty of edge cases in this area:
- interrupted test run;
- did not run because of serial mode failure;
- failed before `test.skip()` call (e.g. in `beforeEach`) in one of the
retries;
- and more...

Related issues: #28322, #28321, #27455, #17652.
Prior changes: #27762, #26385, #28360, probably more.

There is still some duplication between `outcome()` and similar logic in
`base.ts`, which might be deduped in a follow-up.

Fixes #28322.

packages/playwright/src/common/DEPS.list

@@ -2,6 +2,7 @@
 ../util.ts
 ../utilsBundle.ts
 ../transform
+../isomorphic/teleReceiver.ts
 
 [testType.ts]
 ../matchers/expect.ts

packages/playwright/src/common/test.ts

@@ -20,6 +20,7 @@ import type { TestTypeImpl } from './testType';
 import { rootTestType } from './testType';
 import type { Annotation, FixturesWithLocation, FullProjectInternal } from './config';
 import type { Location, FullProject } from '../../types/testReporter';
+import { computeTestCaseOutcome } from '../isomorphic/teleReceiver';
 
 class Base {
   title: string;
@@ -280,21 +281,7 @@ export class TestCase extends Base implements reporterTypes.TestCase {
   }
 
   outcome(): 'skipped' | 'expected' | 'unexpected' | 'flaky' {
-    // Ignore initial skips that may be a result of "skipped because previous test in serial mode failed".
+    return computeTestCaseOutcome(this);
-    const results = [...this.results];
-    while (results[0]?.status === 'skipped' || results[0]?.status === 'interrupted')
-      results.shift();
-
-    // All runs were skipped.
-    if (!results.length)
-      return 'skipped';
-
-    const failures = results.filter(result => result.status !== 'skipped' && result.status !== 'interrupted' && result.status !== this.expectedStatus);
-    if (!failures.length) // all passed
-      return 'expected';
-    if (failures.length === results.length) // all failed
-      return 'unexpected';
-    return 'flaky'; // mixed bag
   }
 
   ok(): boolean {

packages/playwright/src/isomorphic/teleReceiver.ts

@@ -494,21 +494,7 @@ export class TeleTestCase implements reporterTypes.TestCase {
   }
 
   outcome(): 'skipped' | 'expected' | 'unexpected' | 'flaky' {
-    // Ignore initial skips that may be a result of "skipped because previous test in serial mode failed".
+    return computeTestCaseOutcome(this);
-    const results = [...this.results];
-    while (results[0]?.status === 'skipped' || results[0]?.status === 'interrupted')
-      results.shift();
-
-    // All runs were skipped.
-    if (!results.length)
-      return 'skipped';
-
-    const failures = results.filter(result => result.status !== 'skipped' && result.status !== 'interrupted' && result.status !== this.expectedStatus);
-    if (!failures.length) // all passed
-      return 'expected';
-    if (failures.length === results.length) // all failed
-      return 'unexpected';
-    return 'flaky'; // mixed bag
   }
 
   ok(): boolean {
@@ -643,3 +629,47 @@ export function parseRegexPatterns(patterns: JsonPattern[]): (string | RegExp)[]
     return new RegExp(p.r!.source, p.r!.flags);
   });
 }
+
+export function computeTestCaseOutcome(test: reporterTypes.TestCase) {
+  let skipped = 0;
+  let didNotRun = 0;
+  let expected = 0;
+  let interrupted = 0;
+  let unexpected = 0;
+  for (const result of test.results) {
+    if (result.status === 'interrupted') {
+      ++interrupted; // eslint-disable-line @typescript-eslint/no-unused-vars
+    } else if (result.status === 'skipped' && test.expectedStatus === 'skipped') {
+      // Only tests "expected to be skipped" are skipped. These were specifically
+      // marked with test.skip or test.fixme.
+      ++skipped;
+    } else if (result.status === 'skipped') {
+      // Tests that were expected to run, but were skipped are "did not run".
+      // This happens when:
+      // - testing finished early;
+      // - test failure prevented other tests in the serial suite to run;
+      // - probably more cases!
+      ++didNotRun; // eslint-disable-line @typescript-eslint/no-unused-vars
+    } else if (result.status === test.expectedStatus) {
+      // Either passed and expected to pass, or failed and expected to fail.
+      ++expected;
+    } else {
+      ++unexpected;
+    }
+  }
+
+  // Tests that were "skipped as expected" are considered equal to "expected" below,
+  // because that's the expected outcome.
+  //
+  // However, we specifically differentiate the case of "only skipped"
+  // and show it as "skipped" in all reporters.
+  //
+  // More exotic cases like "failed on first run and skipped on retry" are flaky.
+  if (expected === 0 && unexpected === 0)
+    return 'skipped';  // all results were skipped or interrupted
+  if (unexpected === 0)
+    return 'expected';  // no failures, just expected+skipped
+  if (expected === 0 && skipped === 0)
+    return 'unexpected';  // only failures
+  return 'flaky';  // expected+unexpected or skipped+unexpected
+}

packages/playwright/src/reporters/base.ts

@@ -216,7 +216,7 @@ export class BaseReporter implements ReporterV2 {
             if (test.results.some(result => !!result.error))
               interruptedToPrint.push(test);
             interrupted.push(test);
-          } else if (!test.results.length) {
+          } else if (!test.results.length || test.expectedStatus !== 'skipped') {
             ++didNotRun;
           } else {
             ++skipped;

tests/playwright-test/hooks.spec.ts

@@ -392,14 +392,14 @@ test('beforeAll failure should prevent the test, but not afterAll', async ({ run
       test('failed', () => {
         console.log('\\n%%test1');
       });
-      test('skipped', () => {
+      test('does not run', () => {
         console.log('\\n%%test2');
       });
     `,
   });
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(1);
+  expect(result.didNotRun).toBe(1);
   expect(result.outputLines).toEqual([
     'beforeAll',
     'afterAll',
@@ -492,14 +492,14 @@ test('beforeAll timeout should be reported and prevent more tests', async ({ run
       test('failed', () => {
         console.log('\\n%%test1');
       });
-      test('skipped', () => {
+      test('does not run', () => {
         console.log('\\n%%test2');
       });
     `,
   }, { timeout: 1000 });
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(1);
+  expect(result.didNotRun).toBe(1);
   expect(result.outputLines).toEqual([
     'beforeAll',
     'afterAll',
@@ -681,14 +681,14 @@ test('unhandled rejection during beforeAll should be reported and prevent more t
       test('failed', () => {
         console.log('\\n%%test1');
       });
-      test('skipped', () => {
+      test('does not run', () => {
         console.log('\\n%%test2');
       });
     `,
   });
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(1);
+  expect(result.didNotRun).toBe(1);
   expect(result.outputLines).toEqual([
     'beforeAll',
     'afterAll',
@@ -790,7 +790,7 @@ test('beforeAll failure should only prevent tests that are affected', async ({ r
         test('failed', () => {
           console.log('\\n%%test1');
         });
-        test('skipped', () => {
+        test('does not run', () => {
           console.log('\\n%%test2');
         });
       });
@@ -801,7 +801,7 @@ test('beforeAll failure should only prevent tests that are affected', async ({ r
   });
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(1);
+  expect(result.didNotRun).toBe(1);
   expect(result.passed).toBe(1);
   expect(result.outputLines).toEqual([
     'beforeAll',

tests/playwright-test/retry.spec.ts

@@ -216,7 +216,7 @@ test('should retry beforeAll failure', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(0);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(1);
+  expect(result.didNotRun).toBe(1);
   expect(result.output.split('\n')[2]).toBe('×°×°F°');
   expect(result.output).toContain('BeforeAll is bugged!');
 });

tests/playwright-test/runner.spec.ts

@@ -103,7 +103,7 @@ test('should report subprocess creation error', async ({ runInlineTest }, testIn
     'a.spec.js': `
       import { test, expect } from '@playwright/test';
       test('fails', () => {});
-      test('skipped', () => {});
+      test('does not run', () => {});
       // Infect subprocesses to immediately exit when spawning a worker.
       process.env.NODE_OPTIONS = '--require ${JSON.stringify(testInfo.outputPath('preload.js').replace(/\\/g, '\\\\'))}';
     `
@@ -111,7 +111,7 @@ test('should report subprocess creation error', async ({ runInlineTest }, testIn
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(0);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(1);
+  expect(result.didNotRun).toBe(1);
   expect(result.output).toContain('Error: worker process exited unexpectedly (code=42, signal=null)');
 });
 

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

@@ -677,13 +677,14 @@ test('static modifiers should be added in serial mode', async ({ runInlineTest }
       });
       test.skip('skipped', async ({}) => {
       });
-      test('ignored', async ({}) => {
+      test('does not run', async ({}) => {
       });
     `,
   });
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(0);
-  expect(result.skipped).toBe(3);
+  expect(result.skipped).toBe(2);
+  expect(result.didNotRun).toBe(1);
   expect(result.report.suites[0].specs[0].tests[0].annotations).toEqual([{ type: 'slow' }]);
   expect(result.report.suites[0].specs[1].tests[0].annotations).toEqual([{ type: 'fixme' }]);
   expect(result.report.suites[0].specs[2].tests[0].annotations).toEqual([{ type: 'skip' }]);

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

@@ -47,7 +47,7 @@ test('test.describe.serial should work', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(2);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(2);
+  expect(result.didNotRun).toBe(2);
   expect(result.outputLines).toEqual([
     'test1',
     'test2',
@@ -87,7 +87,7 @@ test('test.describe.serial should work in describe', async ({ runInlineTest }) =
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(2);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(2);
+  expect(result.didNotRun).toBe(2);
   expect(result.outputLines).toEqual([
     'test1',
     'test2',
@@ -128,7 +128,7 @@ test('test.describe.serial should work with retry', async ({ runInlineTest }) =>
   expect(result.passed).toBe(2);
   expect(result.flaky).toBe(1);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(1);
+  expect(result.didNotRun).toBe(1);
   expect(result.outputLines).toEqual([
     'test1',
     'test2',
@@ -272,7 +272,7 @@ test('test.describe.serial should work with test.fail', async ({ runInlineTest }
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(2);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(1);
+  expect(result.didNotRun).toBe(1);
   expect(result.outputLines).toEqual([
     'zero',
     'one',
@@ -394,3 +394,55 @@ test('test.describe.serial should work with fullyParallel', async ({ runInlineTe
     'two',
   ]);
 });
+
+test('serial fail + skip is failed', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      import { test, expect } from '@playwright/test';
+      test.describe.configure({ mode: 'serial', retries: 1 });
+      test.describe.serial('serial suite', () => {
+        test('one', async () => {
+          expect(test.info().retry).toBe(0);
+        });
+        test('two', async () => {
+          expect(1).toBe(2);
+        });
+        test('three', async () => {
+        });
+      });
+    `,
+  }, { workers: 1 });
+  expect(result.exitCode).toBe(1);
+  expect(result.passed).toBe(0);
+  expect(result.skipped).toBe(0);
+  expect(result.flaky).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.interrupted).toBe(0);
+  expect(result.didNotRun).toBe(1);
+});
+
+test('serial skip + fail is failed', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      import { test, expect } from '@playwright/test';
+      test.describe.configure({ mode: 'serial', retries: 1 });
+      test.describe.serial('serial suite', () => {
+        test('one', async () => {
+          expect(test.info().retry).toBe(1);
+        });
+        test('two', async () => {
+          expect(1).toBe(2);
+        });
+        test('three', async () => {
+        });
+      });
+    `,
+  }, { workers: 1 });
+  expect(result.exitCode).toBe(1);
+  expect(result.passed).toBe(0);
+  expect(result.skipped).toBe(0);
+  expect(result.flaky).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.interrupted).toBe(0);
+  expect(result.didNotRun).toBe(1);
+});