fix(test runner): extract FailureTracker helper (#26757)

This way we can reuse it for:
- tracking `maxFailures` across phases;
- tracking failures for runner;
- tracking failures for `runJob` helper class later on.

Fixes #26344.

packages/playwright-test/src/runner/dispatcher.ts

@@ -25,6 +25,7 @@ import { WorkerHost } from './workerHost';
 import type { TestGroup } from './testGroups';
 import type { FullConfigInternal } from '../common/config';
 import type { ReporterV2 } from '../reporters/reporterV2';
+import type { FailureTracker } from './failureTracker';
 
 type TestResultData = {
   result: TestResult;
@@ -47,15 +48,15 @@ export class Dispatcher {
   private _testById = new Map<string, TestData>();
   private _config: FullConfigInternal;
   private _reporter: ReporterV2;
-  private _hasWorkerErrors = false;
+  private _failureTracker: FailureTracker;
-  private _failureCount = 0;
 
   private _extraEnvByProjectId: EnvByProjectId = new Map();
   private _producedEnvByProjectId: EnvByProjectId = new Map();
 
-  constructor(config: FullConfigInternal, reporter: ReporterV2) {
+  constructor(config: FullConfigInternal, reporter: ReporterV2, failureTracker: FailureTracker) {
     this._config = config;
     this._reporter = reporter;
+    this._failureTracker = failureTracker;
   }
 
   private _processFullySkippedJobs() {
@@ -196,6 +197,9 @@ export class Dispatcher {
     }
     this._isStopped = false;
     this._workerSlots = [];
+    // 0. Stop right away if we have reached max failures.
+    if (this._failureTracker.hasReachedMaxFailures())
+      void this.stop();
     // 1. Allocate workers.
     for (let i = 0; i < this._config.config.workers; i++)
       this._workerSlots.push({ busy: false });
@@ -248,7 +252,7 @@ export class Dispatcher {
     const onTestEnd = (params: TestEndPayload) => {
       runningTest = false;
       remainingByTestId.delete(params.testId);
-      if (this._hasReachedMaxFailures()) {
+      if (this._failureTracker.hasReachedMaxFailures()) {
         // Do not show more than one error to avoid confusion, but report
         // as interrupted to indicate that we did actually start the test.
         params.status = 'interrupted';
@@ -352,7 +356,7 @@ export class Dispatcher {
         remaining = remaining.filter(test => {
           if (!testIds.has(test.id))
             return true;
-          if (!this._hasReachedMaxFailures()) {
+          if (!this._failureTracker.hasReachedMaxFailures()) {
             const data = this._testById.get(test.id)!;
             const runData = data.resultByWorkerIndex.get(worker.workerIndex);
             // There might be a single test that has started but has not finished yet.
@@ -377,7 +381,7 @@ export class Dispatcher {
         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._hasWorkerErrors = true;
+          this._failureTracker.onWorkerError();
           for (const error of errors)
             this._reporter.onError(error);
         }
@@ -496,7 +500,7 @@ export class Dispatcher {
       this._reporter.onStdErr(chunk, test, result);
     });
     worker.on('teardownErrors', (params: TeardownErrorsPayload) => {
-      this._hasWorkerErrors = true;
+      this._failureTracker.onWorkerError();
       for (const error of params.fatalErrors)
         this._reporter.onError(error);
     });
@@ -519,23 +523,12 @@ export class Dispatcher {
     this._checkFinished();
   }
 
-  private _hasReachedMaxFailures() {
-    const maxFailures = this._config.config.maxFailures;
-    return maxFailures > 0 && this._failureCount >= maxFailures;
-  }
-
   private _reportTestEnd(test: TestCase, result: TestResult) {
-    if (result.status !== 'skipped' && result.status !== test.expectedStatus)
-      ++this._failureCount;
     this._reporter.onTestEnd(test, result);
-    const maxFailures = this._config.config.maxFailures;
+    this._failureTracker.onTestEnd(test, result);
-    if (maxFailures && this._failureCount === maxFailures)
+    if (this._failureTracker.hasReachedMaxFailures())
       this.stop().catch(e => {});
   }
-
-  hasWorkerErrors(): boolean {
-    return this._hasWorkerErrors;
-  }
 }
 
 function chunkFromParams(params: TestOutputPayload): string | Buffer {

packages/playwright-test/src/runner/failureTracker.ts

@@ -0,0 +1,54 @@
+/**
+ * Copyright Microsoft Corporation. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type { TestResult } from '../../types/testReporter';
+import type { FullConfigInternal } from '../common/config';
+import type { Suite, TestCase } from '../common/test';
+
+export class FailureTracker {
+  private _failureCount = 0;
+  private _hasWorkerErrors = false;
+  private _rootSuite: Suite | undefined;
+
+  constructor(private _config: FullConfigInternal) {
+  }
+
+  onRootSuite(rootSuite: Suite) {
+    this._rootSuite = rootSuite;
+  }
+
+  onTestEnd(test: TestCase, result: TestResult) {
+    if (result.status !== 'skipped' && result.status !== test.expectedStatus)
+      ++this._failureCount;
+  }
+
+  onWorkerError() {
+    this._hasWorkerErrors = true;
+  }
+
+  hasReachedMaxFailures() {
+    const maxFailures = this._config.config.maxFailures;
+    return maxFailures > 0 && this._failureCount >= maxFailures;
+  }
+
+  hasWorkerErrors() {
+    return this._hasWorkerErrors;
+  }
+
+  result(): 'failed' | 'passed' {
+    return this._hasWorkerErrors || this._rootSuite?.allTests().some(test => !test.ok()) ? 'failed' : 'passed';
+  }
+}

packages/playwright-test/src/runner/runner.ts

@@ -87,9 +87,7 @@ export class Runner {
     }
 
     const taskStatus = await taskRunner.run(testRun, deadline);
-    let status: FullResult['status'] = 'passed';
+    let status: FullResult['status'] = testRun.failureTracker.result();
-    if (testRun.phases.find(p => p.dispatcher.hasWorkerErrors()) || testRun.rootSuite?.allTests().some(test => !test.ok()))
-      status = 'failed';
     if (status === 'passed' && taskStatus !== 'passed')
       status = taskStatus;
     await reporter.onEnd({ status });

packages/playwright-test/src/runner/tasks.ts

@@ -29,6 +29,7 @@ import { collectProjectsAndTestFiles, createRootSuite, loadFileSuites, loadGloba
 import type { Matcher } from '../util';
 import type { Suite } from '../common/test';
 import { buildDependentProjects, buildTeardownToSetupsMap } from './projectUtils';
+import { FailureTracker } from './failureTracker';
 
 const readDirAsync = promisify(fs.readdir);
 
@@ -46,6 +47,7 @@ export type Phase = {
 export class TestRun {
   readonly reporter: ReporterV2;
   readonly config: FullConfigInternal;
+  readonly failureTracker: FailureTracker;
   rootSuite: Suite | undefined = undefined;
   readonly phases: Phase[] = [];
   projects: FullProjectInternal[] = [];
@@ -55,6 +57,7 @@ export class TestRun {
   constructor(config: FullConfigInternal, reporter: ReporterV2) {
     this.config = config;
     this.reporter = reporter;
+    this.failureTracker = new FailureTracker(config);
   }
 }
 
@@ -190,6 +193,7 @@ function createLoadTask(mode: 'out-of-process' | 'in-process', options: { filter
       await collectProjectsAndTestFiles(testRun, !!options.doNotRunTestsOutsideProjectFilter, options.additionalFileMatcher);
       await loadFileSuites(testRun, mode, options.failOnLoadErrors ? errors : softErrors);
       testRun.rootSuite = await createRootSuite(testRun, options.failOnLoadErrors ? errors : softErrors, !!options.filterOnly);
+      testRun.failureTracker.onRootSuite(testRun.rootSuite);
       // Fail when no tests.
       if (options.failOnLoadErrors && !testRun.rootSuite.allTests().length && !testRun.config.cliPassWithNoTests && !testRun.config.config.shard)
         throw new Error(`No tests found`);
@@ -230,7 +234,7 @@ function createPhasesTask(): Task<TestRun> {
           processed.add(project);
         if (phaseProjects.length) {
           let testGroupsInPhase = 0;
-          const phase: Phase = { dispatcher: new Dispatcher(testRun.config, testRun.reporter), projects: [] };
+          const phase: Phase = { dispatcher: new Dispatcher(testRun.config, testRun.reporter, testRun.failureTracker), projects: [] };
           testRun.phases.push(phase);
           for (const project of phaseProjects) {
             const projectSuite = projectToSuite.get(project)!;
@@ -250,7 +254,7 @@ function createPhasesTask(): Task<TestRun> {
 
 function createRunTestsTask(): Task<TestRun> {
   return {
-    setup: async ({ phases }) => {
+    setup: async ({ phases, failureTracker }) => {
       const successfulProjects = new Set<FullProjectInternal>();
       const extraEnvByProjectId: EnvByProjectId = new Map();
       const teardownToSetups = buildTeardownToSetupsMap(phases.map(phase => phase.projects.map(p => p.project)).flat());
@@ -291,7 +295,7 @@ function createRunTestsTask(): Task<TestRun> {
 
         // If the worker broke, fail everything, we have no way of knowing which
         // projects failed.
-        if (!dispatcher.hasWorkerErrors()) {
+        if (!failureTracker.hasWorkerErrors()) {
           for (const { project, projectSuite } of projects) {
             const hasFailedDeps = project.deps.some(p => !successfulProjects.has(p));
             if (!hasFailedDeps && !projectSuite.allTests().some(test => !test.ok()))

packages/playwright-test/src/runner/watchMode.ts

@@ -298,7 +298,7 @@ async function runTests(config: FullConfigInternal, failedTestIdCollector: Set<s
     }
   }
 
-  if (testRun.phases.find(p => p.dispatcher.hasWorkerErrors()) || hasFailedTests)
+  if (testRun.failureTracker.hasWorkerErrors() || hasFailedTests)
     status = 'failed';
   if (status === 'passed' && taskStatus !== 'passed')
     status = taskStatus;

tests/playwright-test/max-failures.spec.ts

@@ -146,3 +146,40 @@ test('max-failures should properly shutdown', async ({ runInlineTest }) => {
   expect(result.failed).toBe(1);
   expect(result.output).toContain('expect(false).toBeTruthy()');
 });
+
+test('max-failures should work across phases', async ({ runInlineTest }) => {
+  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/26344' });
+
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      const config = {
+        testDir: './',
+        maxFailures: 1,
+        projects: [
+          { name: 'a', testMatch: ['example.spec.ts'] },
+          { name: 'b', testMatch: ['example.spec.ts'], dependencies: ['a'] },
+          { name: 'c', testMatch: ['example.spec.ts'], dependencies: ['a'] },
+          { name: 'd', testMatch: ['example.spec.ts'], dependencies: ['b'] },
+        ]
+      };
+      export default config;
+    `,
+    'example.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test('test', () => {
+        const project = test.info().project.name;
+        console.log('running ' + project);
+        if (project === 'c')
+          throw new Error('failed!');
+      });
+    `,
+  }, { workers: 1 });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.passed).toBe(2);
+  expect(result.skipped).toBe(1);
+  expect(result.output).toContain('running a');
+  expect(result.output).toContain('running b');
+  expect(result.output).toContain('running c');
+  expect(result.output).not.toContain('running d');
+});