fix(test runner): do not send entirely skipped test groups to a worker (#20346)

Move TestCase properties calculation from WorkerMain to suite building
phase.

Fixes #20156.

packages/playwright-test/src/dispatcher.ts

@@ -65,7 +65,34 @@ export class Dispatcher {
     }
   }
 
+  private _processFullySkippedJobs() {
+    // If all the tests in a group are skipped, we report them immediately
+    // without sending anything to a worker. This avoids creating unnecessary worker processes.
+    //
+    // However, if there is at least one non-skipped test in a group, we'll send
+    // the whole group to the worker process and report tests in the natural order,
+    // with skipped tests mixed in-between non-skipped. This makes
+    // for a better reporter experience.
+    while (!this._isStopped && this._queue.length) {
+      const group = this._queue[0];
+      const allTestsSkipped = group.tests.every(test => test.expectedStatus === 'skipped');
+      if (!allTestsSkipped)
+        break;
+
+      for (const test of group.tests) {
+        const result = test._appendTestResult();
+        result.status = 'skipped';
+        this._reporter.onTestBegin?.(test, result);
+        test.annotations = [...test._staticAnnotations];
+        this._reportTestEnd(test, result);
+      }
+      this._queue.shift();
+    }
+  }
+
   private async _scheduleJob() {
+    this._processFullySkippedJobs();
+
     // 1. Find a job to run.
     if (this._isStopped || !this._queue.length)
       return;

packages/playwright-test/src/suiteUtils.ts

@@ -49,19 +49,31 @@ export function buildFileSuiteForProject(project: FullProjectInternal, suite: Su
   result.forEachTest((test, suite) => {
     suite._fileId = fileId;
     const repeatEachIndexSuffix = repeatEachIndex ? ` (repeat:${repeatEachIndex})` : '';
+
     // At the point of the query, suite is not yet attached to the project, so we only get file, describe and test titles.
     const testIdExpression = `[project=${project._id}]${test.titlePath().join('\x1e')}${repeatEachIndexSuffix}`;
     const testId = fileId + '-' + calculateSha1(testIdExpression).slice(0, 20);
     test.id = testId;
     test.repeatEachIndex = repeatEachIndex;
     test._projectId = project._id;
+
+    // Inherit properties from parent suites.
     let inheritedRetries: number | undefined;
+    let inheritedTimeout: number | undefined;
     for (let parentSuite: Suite | undefined = suite; parentSuite; parentSuite = parentSuite.parent) {
       test._staticAnnotations.push(...parentSuite._staticAnnotations);
       if (inheritedRetries === undefined && parentSuite._retries !== undefined)
         inheritedRetries = parentSuite._retries;
+      if (inheritedTimeout === undefined && parentSuite._timeout !== undefined)
+        inheritedTimeout = parentSuite._timeout;
     }
     test.retries = inheritedRetries ?? project.retries;
+    test.timeout = inheritedTimeout ?? project.timeout;
+
+    // Skip annotations imply skipped expectedStatus.
+    if (test._staticAnnotations.some(a => a.type === 'skip' || a.type === 'fixme'))
+      test.expectedStatus = 'skipped';
+
     // We only compute / set digest in the runner.
     if (test._poolDigest)
       test._workerHash = `${project._id}-${test._poolDigest}-${repeatEachIndex}`;

packages/playwright-test/src/test.ts

@@ -41,7 +41,6 @@ export class Suite extends Base implements reporterTypes.Suite {
   location?: Location;
   parent?: Suite;
   _use: FixturesWithLocation[] = [];
-  _skipped = false;
   _entries: (Suite | TestCase)[] = [];
   _hooks: { type: 'beforeEach' | 'afterEach' | 'beforeAll' | 'afterAll', fn: Function, location: Location }[] = [];
   _timeout: number | undefined;
@@ -164,7 +163,6 @@ export class Suite extends Base implements reporterTypes.Suite {
       staticAnnotations: this._staticAnnotations.slice(),
       modifiers: this._modifiers.slice(),
       parallelMode: this._parallelMode,
-      skipped: this._skipped,
       hooks: this._hooks.map(h => ({ type: h.type, location: h.location })),
     };
   }
@@ -179,7 +177,6 @@ export class Suite extends Base implements reporterTypes.Suite {
     suite._staticAnnotations = data.staticAnnotations;
     suite._modifiers = data.modifiers;
     suite._parallelMode = data.parallelMode;
-    suite._skipped = data.skipped;
     suite._hooks = data.hooks.map((h: any) => ({ type: h.type, location: h.location, fn: () => { } }));
     return suite;
   }

packages/playwright-test/src/testType.ts

@@ -92,14 +92,8 @@ export class TestTypeImpl {
 
     if (type === 'only')
       test._only = true;
-    if (type === 'skip' || type === 'fixme') {
+    if (type === 'skip' || type === 'fixme')
       test._staticAnnotations.push({ type });
-      test.expectedStatus = 'skipped';
-    }
-    for (let parent: Suite | undefined = suite; parent; parent = parent.parent) {
-      if (parent._skipped)
-        test.expectedStatus = 'skipped';
-    }
   }
 
   private _describe(type: 'default' | 'only' | 'serial' | 'serial.only' | 'parallel' | 'parallel.only' | 'skip' | 'fixme', location: Location, title: string | Function, fn?: Function) {
@@ -124,10 +118,8 @@ export class TestTypeImpl {
       child._parallelMode = 'serial';
     if (type === 'parallel' || type === 'parallel.only')
       child._parallelMode = 'parallel';
-    if (type === 'skip' || type === 'fixme') {
-      child._skipped = true;
+    if (type === 'skip' || type === 'fixme')
       child._staticAnnotations.push({ type });
-    }
 
     for (let parent: Suite | undefined = suite; parent; parent = parent.parent) {
       if (parent._parallelMode === 'serial' && child._parallelMode === 'parallel')

packages/playwright-test/src/workerMain.ts

@@ -279,16 +279,10 @@ export class WorkerMain extends ProcessRunner {
     const reversedSuites = suites.slice().reverse();
     const nextSuites = new Set(getSuites(nextTest));
 
-    // Inherit test.setTimeout() from parent suites, deepest has the priority.
-    for (const suite of reversedSuites) {
-      if (suite._timeout !== undefined) {
-        testInfo._timeoutManager.setTimeout(suite._timeout);
-        break;
-      }
-    }
-
+    testInfo._timeoutManager.setTimeout(test.timeout);
     for (const annotation of test._staticAnnotations)
       processAnnotation(annotation);
+
     // Process existing annotations dynamically set for parent suites.
     for (const suite of suites) {
       const extraAnnotations = this._extraSuiteAnnotations.get(suite) || [];

tests/playwright-test/playwright-test-fixtures.ts

@@ -211,6 +211,8 @@ async function runPlaywrightCommand(childProcess: CommonFixtures['childProcess']
       PW_TEST_REPORTER_WS_ENDPOINT: undefined,
       PW_TEST_SOURCE_TRANSFORM: undefined,
       PW_TEST_SOURCE_TRANSFORM_SCOPE: undefined,
+      TEST_WORKER_INDEX: undefined,
+      TEST_PARLLEL_INDEX: undefined,
       NODE_OPTIONS: undefined,
       ...env,
     },

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

@@ -543,3 +543,54 @@ test('should not run hooks if modifier throws', async ({ runInlineTest }) => {
     '%%modifier',
   ]);
 });
+
+test('should report skipped tests in-order with correct properties', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'reporter.ts': `
+      class Reporter {
+        onTestBegin(test) {
+          console.log('\\n%%begin-' + test.title);
+        }
+        onTestEnd(test, result) {
+          console.log('\\n%%end-' + test.title);
+          console.log('\\n%%expectedStatus-' + test.expectedStatus);
+          console.log('\\n%%timeout-' + test.timeout);
+          console.log('\\n%%retries-' + test.retries);
+        }
+      }
+      export default Reporter;
+    `,
+    'playwright.config.ts': `
+      module.exports = { reporter: [['./reporter.ts']] };
+    `,
+    'a.test.ts': `
+      const { test } = pwt;
+      test.describe.configure({ timeout: 1234, retries: 3 });
+      test('test1', async ({}) => {
+      });
+      test.skip('test2', async ({}) => {
+      });
+      test('test3', async ({}) => {
+      });
+    `,
+  }, { reporter: '', workers: 1 });
+
+  expect(result.exitCode).toBe(0);
+  expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([
+    '%%begin-test1',
+    '%%end-test1',
+    '%%expectedStatus-passed',
+    '%%timeout-1234',
+    '%%retries-3',
+    '%%begin-test2',
+    '%%end-test2',
+    '%%expectedStatus-skipped',
+    '%%timeout-1234',
+    '%%retries-3',
+    '%%begin-test3',
+    '%%end-test3',
+    '%%expectedStatus-passed',
+    '%%timeout-1234',
+    '%%retries-3',
+  ]);
+});

tests/playwright-test/worker-index.spec.ts

@@ -219,3 +219,23 @@ test('parallelIndex should be in 0..workers-1', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(0);
   expect(result.passed).toBe(20);
 });
+
+test('should not spawn workers for statically skipped tests', async ({ runInlineTest }) => {
+  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/20156' });
+
+  const result = await runInlineTest({
+    'a.test.js': `
+      console.log('%%workerIndex=' + process.env.TEST_WORKER_INDEX);
+      const { test } = pwt;
+      test.describe.configure({ mode: 'parallel' });
+      test('success', () => {});
+      test.skip('skipped', () => {});
+    `,
+  }, { workers: 2 });
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(1);
+  expect(result.skipped).toBe(1);
+  expect(result.output).toContain('workerIndex=undefined');
+  expect(result.output).toContain('workerIndex=0');
+  expect(result.output).not.toContain('workerIndex=1');
+});