From 03fe75251bd3c410e988db73fa1c58ddad3e0b2f Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Wed, 3 Aug 2022 15:25:25 -0700
Subject: [PATCH] fix(test runner): show tests as interrupted when maxFailures
 stops them (#16178)

Previously, we marked these tests as skipped, and it was sometimes
confusing, because they did actually run and produce some output/artifacts.
---
 packages/playwright-test/src/dispatcher.ts     | 10 ++++++----
 tests/playwright-test/max-failures.spec.ts     |  5 +++--
 tests/playwright-test/playwright.trace.spec.ts |  6 +++---
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/packages/playwright-test/src/dispatcher.ts b/packages/playwright-test/src/dispatcher.ts
index 0971be701b..3026389ea8 100644
--- a/packages/playwright-test/src/dispatcher.ts
+++ b/packages/playwright-test/src/dispatcher.ts
@@ -196,8 +196,6 @@ export class Dispatcher {
 
     const onTestBegin = (params: TestBeginPayload) => {
       const data = this._testById.get(params.testId)!;
-      if (this._hasReachedMaxFailures())
-        return;
       const result = data.test._appendTestResult();
       data.resultByWorkerIndex.set(worker.workerIndex, { result, stepStack: new Set(), steps: new Map() });
       result.workerIndex = worker.workerIndex;
@@ -208,8 +206,12 @@ export class Dispatcher {
 
     const onTestEnd = (params: TestEndPayload) => {
       remainingByTestId.delete(params.testId);
-      if (this._hasReachedMaxFailures())
-        return;
+      if (this._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';
+        params.errors = [];
+      }
       const data = this._testById.get(params.testId)!;
       const test = data.test;
       const { result } = data.resultByWorkerIndex.get(worker.workerIndex)!;
diff --git a/tests/playwright-test/max-failures.spec.ts b/tests/playwright-test/max-failures.spec.ts
index b6463eb5f8..a06b481e4a 100644
--- a/tests/playwright-test/max-failures.spec.ts
+++ b/tests/playwright-test/max-failures.spec.ts
@@ -98,7 +98,7 @@ test('max-failures should stop workers', async ({ runInlineTest }) => {
       test('passed short', async () => {
         await new Promise(f => setTimeout(f, 1));
       });
-      test('interrupted counts as skipped', async () => {
+      test('interrupted reported as interrupted', async () => {
         console.log('\\n%%interrupted');
         await new Promise(f => setTimeout(f, 5000));
       });
@@ -110,7 +110,8 @@ test('max-failures should stop workers', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(1);
   expect(result.passed).toBe(2);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(2);
+  expect(result.interrupted).toBe(1);
+  expect(result.skipped).toBe(1);
   expect(result.output).toContain('%%interrupted');
   expect(result.output).not.toContain('%%skipped');
 });
diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts
index b7880f0eee..a247b8186a 100644
--- a/tests/playwright-test/playwright.trace.spec.ts
+++ b/tests/playwright-test/playwright.trace.spec.ts
@@ -241,7 +241,7 @@ test('should not override trace file in afterAll', async ({ runInlineTest, serve
   expect(fs.existsSync(testInfo.outputPath('test-results', 'a-test-1', 'trace-1.zip'))).toBeTruthy();
 });
 
-test.fixme('should not retain traces for interrupted tests', async ({ runInlineTest }, testInfo) => {
+test('should retain traces for interrupted tests', async ({ runInlineTest }, testInfo) => {
   const result = await runInlineTest({
     'playwright.config.ts': `
       module.exports = { use: { trace: 'retain-on-failure' }, maxFailures: 1 };
@@ -261,9 +261,9 @@ test.fixme('should not retain traces for interrupted tests', async ({ runInlineT
 
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
-  expect(result.skipped).toBe(1);
+  expect(result.interrupted).toBe(1);
   expect(fs.existsSync(testInfo.outputPath('test-results', 'a-test-1', 'trace.zip'))).toBeTruthy();
-  expect(fs.existsSync(testInfo.outputPath('test-results', 'b-test-2', 'trace.zip'))).toBeFalsy();
+  expect(fs.existsSync(testInfo.outputPath('test-results', 'b-test-2', 'trace.zip'))).toBeTruthy();
 });
 
 async function parseTrace(file: string): Promise<Map<string, Buffer>> {