chore(test runner): simplify some dispatcher logic (#27732)

- remove `onlyStartedTests` in favor of explicit branch with comments;
- produce one "test not found" error per test instead of a single large
error;
- extract `_failTestWithErrors` from `_massSkipTestsFromRemaining`.

packages/playwright/src/runner/dispatcher.ts

@@ -357,27 +357,29 @@ class JobDispatcher {
     data.result.attachments.push(attachment);
   }
 
-  private _massSkipTestsFromRemaining(testIds: Set<string>, errors: TestError[], onlyStartedTests?: boolean) {
+  private _failTestWithErrors(test: TestCase, errors: TestError[]) {
+    const runData = this._dataByTestId.get(test.id);
+    // There might be a single test that has started but has not finished yet.
+    let result: TestResult;
+    if (runData) {
+      result = runData.result;
+    } else {
+      result = test._appendTestResult();
+      this._reporter.onTestBegin(test, result);
+    }
+    result.errors = [...errors];
+    result.error = result.errors[0];
+    result.status = errors.length ? 'failed' : 'skipped';
+    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()) {
-        const runData = this._dataByTestId.get(test.id);
+        this._failTestWithErrors(test, errors);
-        // There might be a single test that has started but has not finished yet.
-        let result: TestResult;
-        if (runData) {
-          result = runData.result;
-        } else {
-          if (onlyStartedTests && this._currentlyRunning)
-            continue;
-          result = test._appendTestResult();
-          this._reporter.onTestBegin(test, result);
-        }
-        result.errors = [...errors];
-        result.error = result.errors[0];
-        result.status = errors.length ? 'failed' : 'skipped';
-        this._reportTestEnd(test, result);
-        this._failedTests.add(test);
         errors = []; // Only report errors for the first test.
       }
       this._remainingByTestId.delete(test.id);
@@ -401,15 +403,14 @@ class JobDispatcher {
       return;
     }
 
-    if (params.fatalUnknownTestIds) {
+    for (const testId of params.fatalUnknownTestIds || []) {
-      const titles = params.fatalUnknownTestIds.map(testId => {
+      const test = this._remainingByTestId.get(testId);
-        const test = this._remainingByTestId.get(testId);
+      if (test) {
-        return test?.titlePath().slice(1).join(' > ');
+        this._remainingByTestId.delete(testId);
-      }).filter(title => !!title);
+        this._failTestWithErrors(test, [{ message: `Test not found in the worker process. Make sure test title does not change.` }]);
-      this._massSkipTestsFromRemaining(new Set(params.fatalUnknownTestIds), [{
+      }
-        message: `Test(s) not found in the worker process. Make sure test titles do not change:\n${titles.join('\n')}`
-      }]);
     }
+
     if (params.fatalErrors.length) {
       // In case of fatal errors, report first remaining test as failing with these errors,
       // and all others as skipped.
@@ -417,9 +418,18 @@ class JobDispatcher {
     }
     // Handle tests that should be skipped because of the setup failure.
     this._massSkipTestsFromRemaining(new Set(params.skipTestsDueToSetupFailure), []);
-    // Handle unexpected worker exit.
+
-    if (params.unexpectedExitError)
+    if (params.unexpectedExitError) {
-      this._massSkipTestsFromRemaining(new Set(this._remainingByTestId.keys()), [params.unexpectedExitError], true /* onlyStartedTests */);
+      // 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]);
+    }
 
     const retryCandidates = new Set<TestCase>();
     const serialSuitesWithFailures = new Set<Suite>();

tests/playwright-test/runner.spec.ts

@@ -16,7 +16,7 @@
 
 import fs from 'fs';
 import path from 'path';
-import { test, expect, parseTestRunnerOutput } from './playwright-test-fixtures';
+import { test, expect, parseTestRunnerOutput, countTimes } from './playwright-test-fixtures';
 
 test('it should not allow multiple tests with the same name per suite', async ({ runInlineTest }) => {
   const result = await runInlineTest({
@@ -377,8 +377,6 @@ test('should teardown workers that are redundant', async ({ runInlineTest }) =>
 });
 
 test('should not hang if test suites in worker are inconsistent with runner', async ({ runInlineTest }) => {
-  const oldValue = process.env.TEST_WORKER_INDEX;
-  delete process.env.TEST_WORKER_INDEX;
   const result = await runInlineTest({
     'playwright.config.ts': `
       module.exports = { name: 'project-name' };
@@ -400,13 +398,13 @@ test('should not hang if test suites in worker are inconsistent with runner', as
         });
       }
     `,
-  }, { 'workers': 1 });
+  }, { 'workers': 1 }, { TEST_WORKER_INDEX: undefined });
-  process.env.TEST_WORKER_INDEX = oldValue;
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(1);
-  expect(result.failed).toBe(1);
+  expect(result.failed).toBe(2);
-  expect(result.skipped).toBe(1);
+  expect(result.skipped).toBe(0);
-  expect(result.report.suites[0].specs[1].tests[0].results[0].error!.message).toBe('Test(s) not found in the worker process. Make sure test titles do not change:\nproject-name > a.spec.js > Test 1 - bar\nproject-name > a.spec.js > Test 2 - baz');
+  const expectedError = 'Test not found in the worker process. Make sure test title does not change.';
+  expect(countTimes(result.output, expectedError)).toBe(2);  // Once per each test that was missing.
 });
 
 test('sigint should stop global setup', async ({ interactWithTestRunner }) => {