chore(test runner): remove fake skipped test results (microsoft#27762)

Fixes microsoft#27455.

packages/playwright/src/common/test.ts

@@ -239,6 +239,9 @@ export class TestCase extends Base implements reporterTypes.TestCase {
   _poolDigest = '';
   _workerHash = '';
   _projectId = '';
+  // This is different from |results.length| because sometimes we do not run the test, but consume
+  // an attempt, for example when skipping tests in a serial suite after a failure.
+  _runAttempts = 0;
   // Annotations known statically before running the test, e.g. `test.skip()` or `test.describe.skip()`.
   _staticAnnotations: Annotation[] = [];
 
@@ -256,16 +259,10 @@ 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".
-    const results = [...this.results];
-    while (results[0]?.status === 'skipped' || results[0]?.status === 'interrupted')
-      results.shift();
-
-    // All runs were skipped.
-    if (!results.length)
+    const results = this.results.filter(result => result.status !== 'interrupted');
+    if (results.every(result => result.status === 'skipped'))
       return 'skipped';
-
-    const failures = results.filter(result => result.status !== 'skipped' && result.status !== 'interrupted' && result.status !== this.expectedStatus);
+    const failures = results.filter(result => result.status !== this.expectedStatus);
     if (!failures.length) // all passed
       return 'expected';
     if (failures.length === results.length) // all failed

packages/playwright/src/isomorphic/teleReceiver.ts

@@ -493,16 +493,10 @@ 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".
-    const results = [...this.results];
-    while (results[0]?.status === 'skipped' || results[0]?.status === 'interrupted')
-      results.shift();
-
-    // All runs were skipped.
-    if (!results.length)
+    const results = this.results.filter(result => result.status !== 'interrupted');
+    if (results.every(result => result.status === 'skipped'))
       return 'skipped';
-
-    const failures = results.filter(result => result.status !== 'skipped' && result.status !== 'interrupted' && result.status !== this.expectedStatus);
+    const failures = results.filter(result => result.status !== this.expectedStatus);
     if (!failures.length) // all passed
       return 'expected';
     if (failures.length === results.length) // all failed

packages/playwright/src/runner/dispatcher.ts

@@ -290,7 +290,7 @@ class JobDispatcher {
     test.expectedStatus = params.expectedStatus;
     test.annotations = params.annotations;
     test.timeout = params.timeout;
-    const isFailure = result.status !== 'skipped' && result.status !== test.expectedStatus;
+    const isFailure = result.status !== test.expectedStatus;
     if (isFailure)
       this._failedTests.add(test);
     this._reportTestEnd(test, result);
@@ -369,22 +369,17 @@ class JobDispatcher {
     }
     result.errors = [...errors];
     result.error = result.errors[0];
-    result.status = errors.length ? 'failed' : 'skipped';
+    result.status = 'failed';
     this._reportTestEnd(test, result);
     this._failedTests.add(test);
   }
 
-  private _massSkipTestsFromRemaining(testIds: Set<string>, errors: TestError[]) {
-    for (const test of this._remainingByTestId.values()) {
-      if (!testIds.has(test.id))
-        continue;
-      if (!this._failureTracker.hasReachedMaxFailures()) {
-        this._failTestWithErrors(test, errors);
-        errors = []; // Only report errors for the first test.
-      }
+  private _handleFatalErrors(errors: TestError[]) {
+    const test = this._remainingByTestId.values().next().value as TestCase | undefined;
+    if (test) {
+      this._failTestWithErrors(test, errors);
       this._remainingByTestId.delete(test.id);
-    }
-    if (errors.length) {
+    } else if (errors.length) {
       // We had fatal errors after all tests have passed - most likely in some teardown.
       // Let's just fail the test run.
       this._failureTracker.onWorkerError();
@@ -412,23 +407,28 @@ class JobDispatcher {
     }
 
     if (params.fatalErrors.length) {
-      // In case of fatal errors, report first remaining test as failing with these errors,
-      // and all others as skipped.
-      this._massSkipTestsFromRemaining(new Set(this._remainingByTestId.keys()), params.fatalErrors);
+      // In case of fatal errors, report the first remaining test as failing with these errors,
+      // and "skip" all other tests to avoid running into the same issue over and over.
+      this._handleFatalErrors(params.fatalErrors);
+      this._remainingByTestId.clear();
     }
+
     // Handle tests that should be skipped because of the setup failure.
-    this._massSkipTestsFromRemaining(new Set(params.skipTestsDueToSetupFailure), []);
+    for (const testId of params.skipTestsDueToSetupFailure)
+      this._remainingByTestId.delete(testId);
 
     if (params.unexpectedExitError) {
-      // When worker exits during a test, we blame the test itself.
-      //
-      // The most common situation when worker exits while not running a test is:
-      //   worker failed to require the test file (at the start) because of an exception in one of imports.
-      // In this case, "skip" all remaining tests, to avoid running into the same exception over and over.
-      if (this._currentlyRunning)
-        this._massSkipTestsFromRemaining(new Set([this._currentlyRunning.test.id]), [params.unexpectedExitError]);
-      else
-        this._massSkipTestsFromRemaining(new Set(this._remainingByTestId.keys()), [params.unexpectedExitError]);
+      if (this._currentlyRunning) {
+        // When worker exits during a test, we blame the test itself.
+        this._failTestWithErrors(this._currentlyRunning.test, [params.unexpectedExitError]);
+        this._remainingByTestId.delete(this._currentlyRunning.test.id);
+      } else {
+        // The most common situation when worker exits while not running a test is:
+        //   worker failed to require the test file (at the start) because of an exception in one of imports.
+        // In this case, "skip" all remaining tests, to avoid running into the same exception over and over.
+        this._handleFatalErrors([params.unexpectedExitError]);
+        this._remainingByTestId.clear();
+      }
     }
 
     const retryCandidates = new Set<TestCase>();
@@ -446,26 +446,22 @@ class JobDispatcher {
         serialSuitesWithFailures.add(outermostSerialSuite);
     }
 
-    // If we have failed tests that belong to a serial suite,
-    // we should skip all future tests from the same serial suite.
-    const testsBelongingToSomeSerialSuiteWithFailures = [...this._remainingByTestId.values()].filter(test => {
-      let parent: Suite | undefined = test.parent;
-      while (parent && !serialSuitesWithFailures.has(parent))
-        parent = parent.parent;
-      return !!parent;
-    });
-    this._massSkipTestsFromRemaining(new Set(testsBelongingToSomeSerialSuiteWithFailures.map(test => test.id)), []);
-
     for (const serialSuite of serialSuitesWithFailures) {
-      // Add all tests from failed serial suites for possible retry.
-      // These will only be retried together, because they have the same
-      // "retries" setting and the same number of previous runs.
-      serialSuite.allTests().forEach(test => retryCandidates.add(test));
+      serialSuite.allTests().forEach(test => {
+        // Skip remaining tests from serial suites with failures.
+        this._remainingByTestId.delete(test.id);
+        // Schedule them for the retry all together.
+        retryCandidates.add(test);
+      });
     }
 
+    for (const test of this._job.tests) {
+      if (!this._remainingByTestId.has(test.id))
+        ++test._runAttempts;
+    }
     const remaining = [...this._remainingByTestId.values()];
     for (const test of retryCandidates) {
-      if (test.results.length < test.retries + 1)
+      if (test._runAttempts < test.retries + 1)
         remaining.push(test);
     }
 

packages/playwright/src/runner/failureTracker.ts

@@ -31,7 +31,7 @@ export class FailureTracker {
   }
 
   onTestEnd(test: TestCase, result: TestResult) {
-    if (result.status !== 'skipped' && result.status !== test.expectedStatus)
+    if (result.status !== test.expectedStatus)
       ++this._failureCount;
   }
 

tests/playwright-test/hooks.spec.ts

@@ -399,7 +399,7 @@ test('beforeAll failure should prevent the test, but not afterAll', async ({ run
   });
   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',
@@ -499,7 +499,7 @@ test('beforeAll timeout should be reported and prevent more tests', async ({ run
   }, { 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',
@@ -688,7 +688,7 @@ test('unhandled rejection during beforeAll should be reported and prevent more t
   });
   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',
@@ -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,8 +216,8 @@ 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.output.split('\n')[2]).toBe('×°×°F°');
+  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

@@ -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)');
 });
 
@@ -634,9 +634,9 @@ test('should not hang on worker error in test file', async ({ runInlineTest }) =
     `,
   }, { 'timeout': 3000 });
   expect(result.exitCode).toBe(1);
+  expect(result.results).toHaveLength(1);
   expect(result.results[0].status).toBe('failed');
   expect(result.results[0].error.message).toContain('Error: worker process exited unexpectedly');
-  expect(result.results[1].status).toBe('skipped');
 });
 
 test('fast double SIGINT should be ignored', async ({ interactWithTestRunner }) => {

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

@@ -640,7 +640,7 @@ test('static modifiers should be added in serial mode', async ({ runInlineTest }
   });
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(0);
-  expect(result.skipped).toBe(3);
+  expect(result.didNotRun).toBe(3);
   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);
+});