fix(test-runner): sharding tests does not show a lot of skips (#7708)

src/test/dispatcher.ts

@@ -19,9 +19,10 @@ import path from 'path';
 import { EventEmitter } from 'events';
 import { RunPayload, TestBeginPayload, TestEndPayload, DonePayload, TestOutputPayload, WorkerInitParams } from './ipc';
 import type { TestResult, Reporter, TestStatus } from '../../types/testReporter';
-import { Suite, TestCase } from './test';
+import { TestCase } from './test';
 import { Loader } from './loader';
 
+// TODO: use TestGroup instead of DispatcherEntry
 type DispatcherEntry = {
   runPayload: RunPayload;
   hash: string;
@@ -29,6 +30,14 @@ type DispatcherEntry = {
   projectIndex: number;
 };
 
+export type TestGroup = {
+  workerHash: string;
+  requireFile: string;
+  repeatEachIndex: number;
+  projectIndex: number;
+  tests: TestCase[];
+};
+
 export class Dispatcher {
   private _workers = new Set<Worker>();
   private _freeWorkers: Worker[] = [];
@@ -38,82 +47,36 @@ export class Dispatcher {
   private _queue: DispatcherEntry[] = [];
   private _stopCallback = () => {};
   readonly _loader: Loader;
-  private _suite: Suite;
   private _reporter: Reporter;
   private _hasWorkerErrors = false;
   private _isStopped = false;
   private _failureCount = 0;
 
-  constructor(loader: Loader, suite: Suite, reporter: Reporter) {
+  constructor(loader: Loader, testGroups: TestGroup[], reporter: Reporter) {
     this._loader = loader;
     this._reporter = reporter;
 
-    this._suite = suite;
+    this._queue = [];
-    for (const suite of this._suite.suites) {
+    for (const group of testGroups) {
-      for (const test of suite.allTests())
+      const entry: DispatcherEntry = {
-        this._testById.set(test._id, { test, result: test._appendTestResult() });
+        runPayload: {
-    }
+          file: group.requireFile,
-
+          entries: []
-    this._queue = this._filesSortedByWorkerHash();
+        },
-
+        hash: group.workerHash,
-    // Shard tests.
+        repeatEachIndex: group.repeatEachIndex,
-    const shard = this._loader.fullConfig().shard;
+        projectIndex: group.projectIndex,
-    if (shard) {
+      };
-      let total = this._suite.allTests().length;
+      for (const test of group.tests) {
-      const shardSize = Math.ceil(total / shard.total);
+        const result = test._appendTestResult();
-      const from = shardSize * (shard.current - 1);
+        this._testById.set(test._id, { test, result });
-      const to = shardSize * shard.current;
-      let current = 0;
-      total = 0;
-      const filteredQueue: DispatcherEntry[] = [];
-      for (const entry of this._queue) {
-        if (current >= from && current < to) {
-          filteredQueue.push(entry);
-          total += entry.runPayload.entries.length;
-        }
-        current += entry.runPayload.entries.length;
-      }
-      this._queue = filteredQueue;
-    }
-  }
-
-  _filesSortedByWorkerHash(): DispatcherEntry[] {
-    const entriesByWorkerHashAndFile = new Map<string, Map<string, DispatcherEntry>>();
-    for (const projectSuite of this._suite.suites) {
-      for (const test of projectSuite.allTests()) {
-        let entriesByFile = entriesByWorkerHashAndFile.get(test._workerHash);
-        if (!entriesByFile) {
-          entriesByFile = new Map();
-          entriesByWorkerHashAndFile.set(test._workerHash, entriesByFile);
-        }
-        const file = test._requireFile;
-        let entry = entriesByFile.get(file);
-        if (!entry) {
-          entry = {
-            runPayload: {
-              entries: [],
-              file,
-            },
-            repeatEachIndex: test._repeatEachIndex,
-            projectIndex: test._projectIndex,
-            hash: test._workerHash,
-          };
-          entriesByFile.set(file, entry);
-        }
         entry.runPayload.entries.push({
-          retry: this._testById.get(test._id)!.result.retry,
+          retry: result.retry,
           testId: test._id,
         });
       }
+      this._queue.push(entry);
     }
-
-    const result: DispatcherEntry[] = [];
-    for (const entriesByFile of entriesByWorkerHashAndFile.values()) {
-      for (const entry of entriesByFile.values())
-        result.push(entry);
-    }
-    result.sort((a, b) => a.hash < b.hash ? -1 : (a.hash === b.hash ? 0 : 1));
-    return result;
   }
 
   async run() {

src/test/runner.ts

@@ -20,7 +20,7 @@ import rimraf from 'rimraf';
 import * as fs from 'fs';
 import * as path from 'path';
 import { promisify } from 'util';
-import { Dispatcher } from './dispatcher';
+import { Dispatcher, TestGroup } from './dispatcher';
 import { createMatcher, FilePatternFilter, monotonicTime, raceAgainstDeadline } from './util';
 import { TestCase, Suite } from './test';
 import { Loader } from './loader';
@@ -224,12 +224,51 @@ export class Runner {
         outputDirs.add(project.config.outputDir);
       }
 
-      const total = rootSuite.allTests().length;
+      let total = rootSuite.allTests().length;
       if (!total)
         return { status: 'no-tests' };
 
       await Promise.all(Array.from(outputDirs).map(outputDir => removeFolderAsync(outputDir).catch(e => {})));
 
+      let testGroups = createTestGroups(rootSuite);
+
+      const shard = config.shard;
+      if (shard) {
+        const shardGroups: TestGroup[] = [];
+        const shardTests = new Set<TestCase>();
+
+        // Each shard gets some tests.
+        const shardSize = Math.floor(total / shard.total);
+        // First few shards get one more test each.
+        const extraOne = total - shardSize * shard.total;
+
+        const currentShard = shard.current - 1; // Make it zero-based for calculations.
+        const from = shardSize * currentShard + Math.min(extraOne, currentShard);
+        const to = from + shardSize + (currentShard < extraOne ? 1 : 0);
+        let current = 0;
+        for (const group of testGroups) {
+          // Any test group goes to the shard that contains the first test of this group.
+          // So, this shard gets any group that starts at [from; to)
+          if (current >= from && current < to) {
+            shardGroups.push(group);
+            for (const test of group.tests)
+              shardTests.add(test);
+          }
+          current += group.tests.length;
+        }
+
+        testGroups = shardGroups;
+        filterSuite(rootSuite, () => false, test => shardTests.has(test));
+        total = rootSuite.allTests().length;
+      }
+
+      if (process.stdout.isTTY) {
+        console.log();
+        const jobs = Math.min(config.workers, testGroups.length);
+        const shardDetails = shard ? `, shard ${shard.current} of ${shard.total}` : '';
+        console.log(`Running ${total} test${total > 1 ? 's' : ''} using ${jobs} worker${jobs > 1 ? 's' : ''}${shardDetails}`);
+      }
+
       let sigint = false;
       let sigintCallback: () => void;
       const sigIntPromise = new Promise<void>(f => sigintCallback = f);
@@ -244,23 +283,11 @@ export class Runner {
       };
       process.on('SIGINT', sigintHandler);
 
-      if (process.stdout.isTTY) {
-        const workers = new Set();
-        rootSuite.allTests().forEach(test => {
-          workers.add(test._requireFile + test._workerHash);
-        });
-        console.log();
-        const jobs = Math.min(config.workers, workers.size);
-        const shard = config.shard;
-        const shardDetails = shard ? `, shard ${shard.current} of ${shard.total}` : '';
-        console.log(`Running ${total} test${total > 1 ? 's' : ''} using ${jobs} worker${jobs > 1 ? 's' : ''}${shardDetails}`);
-      }
-
       this._reporter.onBegin?.(config, rootSuite);
       this._didBegin = true;
       let hasWorkerErrors = false;
       if (!list) {
-        const dispatcher = new Dispatcher(this._loader, rootSuite, this._reporter);
+        const dispatcher = new Dispatcher(this._loader, testGroups, this._reporter);
         await Promise.race([dispatcher.run(), sigIntPromise]);
         await dispatcher.stop();
         hasWorkerErrors = dispatcher.hasWorkerErrors();
@@ -411,6 +438,30 @@ function buildItemLocation(rootDir: string, testOrSuite: Suite | TestCase) {
   return `${path.relative(rootDir, testOrSuite.location.file)}:${testOrSuite.location.line}`;
 }
 
+function createTestGroups(rootSuite: Suite): TestGroup[] {
+  const groupById = new Map<string, TestGroup>();
+  for (const projectSuite of rootSuite.suites) {
+    for (const test of projectSuite.allTests()) {
+      const id = test._workerHash + '::' + test._requireFile;
+      let group = groupById.get(id);
+      if (!group) {
+        group = {
+          workerHash: test._workerHash,
+          requireFile: test._requireFile,
+          repeatEachIndex: test._repeatEachIndex,
+          projectIndex: test._projectIndex,
+          tests: [],
+        };
+        groupById.set(id, group);
+      }
+      group.tests.push(test);
+    }
+  }
+  const groups = Array.from(groupById.values());
+  groups.sort((a, b) => a.workerHash.localeCompare(b.workerHash));
+  return groups;
+}
+
 class ListModeReporter implements Reporter {
   onBegin(config: FullConfig, suite: Suite): void {
     console.log(`Listing tests:`);

tests/playwright-test/fixtures.spec.ts

@@ -557,7 +557,7 @@ test('should create a new worker for worker fixtures', async ({ runInlineTest })
     'a.test.ts': `
       const { test } = pwt;
       test('base test', async ({}, testInfo) => {
-        expect(testInfo.workerIndex).toBe(0);
+        console.log('\\n%%base-' + testInfo.workerIndex);
       });
 
       const test2 = test.extend({
@@ -567,7 +567,7 @@ test('should create a new worker for worker fixtures', async ({ runInlineTest })
         }, { scope: 'worker' }],
       });
       test2('a test', async ({ foo }, testInfo) => {
-        expect(testInfo.workerIndex).toBe(1);
+        console.log('\\n%%a-' + testInfo.workerIndex);
       });
     `,
     'b.test.ts': `
@@ -579,12 +579,16 @@ test('should create a new worker for worker fixtures', async ({ runInlineTest })
         },
       });
       test2('b test', async ({ bar }, testInfo) => {
-        expect(testInfo.workerIndex).toBe(0);
+        console.log('\\n%%b-' + testInfo.workerIndex);
       });
     `,
   }, { workers: 1 });
   expect(result.output).toContain('foo-a');
   expect(result.output).toContain('bar-b');
+  const baseWorker = +result.output.match(/%%base-(\d)/)[1];
+  expect(result.output).toContain(`%%base-${baseWorker}`);
+  expect(result.output).toContain(`%%a-${1 - baseWorker}`);
+  expect(result.output).toContain(`%%b-${baseWorker}`);
   expect(result.passed).toBe(3);
 });
 

tests/playwright-test/json-reporter.spec.ts

@@ -34,7 +34,7 @@ test('should support spec.ok', async ({ runInlineTest }) => {
   expect(result.report.suites[0].specs[1].ok).toBe(false);
 });
 
-test('should report skipped due to sharding', async ({ runInlineTest }) => {
+test('should not report skipped due to sharding', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.test.js': `
       const { test } = pwt;
@@ -56,11 +56,10 @@ test('should report skipped due to sharding', async ({ runInlineTest }) => {
     `,
   }, { shard: '1/3', reporter: 'json' });
   expect(result.exitCode).toBe(0);
+  expect(result.report.suites.length).toBe(1);
+  expect(result.report.suites[0].specs.length).toBe(2);
   expect(result.report.suites[0].specs[0].tests[0].status).toBe('expected');
   expect(result.report.suites[0].specs[1].tests[0].status).toBe('skipped');
-  expect(result.report.suites[1].specs[0].tests[0].status).toBe('skipped');
-  expect(result.report.suites[1].specs[1].tests[0].status).toBe('skipped');
-  expect(result.report.suites[1].specs[2].tests[0].status).toBe('skipped');
 });
 
 test('should report projects', async ({ runInlineTest }, testInfo) => {

tests/playwright-test/junit-reporter.spec.ts

@@ -179,13 +179,10 @@ test('should report skipped due to sharding', async ({ runInlineTest }) => {
     `,
   }, { shard: '1/3', reporter: 'junit' });
   const xml = parseXML(result.output);
+  expect(xml['testsuites']['testsuite'].length).toBe(1);
   expect(xml['testsuites']['testsuite'][0]['$']['tests']).toBe('2');
   expect(xml['testsuites']['testsuite'][0]['$']['failures']).toBe('0');
   expect(xml['testsuites']['testsuite'][0]['$']['skipped']).toBe('1');
-
-  expect(xml['testsuites']['testsuite'][1]['$']['tests']).toBe('3');
-  expect(xml['testsuites']['testsuite'][1]['$']['failures']).toBe('0');
-  expect(xml['testsuites']['testsuite'][1]['$']['skipped']).toBe('3');
   expect(result.exitCode).toBe(0);
 });
 

tests/playwright-test/shard.spec.ts

@@ -18,22 +18,29 @@ import { test, expect } from './playwright-test-fixtures';
 
 const tests = {
   'a.spec.ts': `
-      const { test } = pwt;
+    const { test } = pwt;
+    test.use({ headless: false });
     test('test1', async () => {
       console.log('test1-done');
     });
-    test('test2', async () => {
+    test.describe('suite', () => {
-      console.log('test2-done');
+      test.use({ headless: true });
+      test('test2', async () => {
+        console.log('test2-done');
+      });
     });
     test('test3', async () => {
       console.log('test3-done');
     });
   `,
   'b.spec.ts': `
-      const { test } = pwt;
+    const { test } = pwt;
     test('test4', async () => {
       console.log('test4-done');
     });
+    test('test5', async () => {
+      console.log('test5-done');
+    });
   `,
 };
 
@@ -41,18 +48,19 @@ test('should respect shard=1/2', async ({ runInlineTest }) => {
   const result = await runInlineTest(tests, { shard: '1/2' });
   expect(result.exitCode).toBe(0);
   expect(result.passed).toBe(3);
-  expect(result.skipped).toBe(1);
+  expect(result.skipped).toBe(0);
-  expect(result.output).toContain('test1-done');
   expect(result.output).toContain('test2-done');
-  expect(result.output).toContain('test3-done');
+  expect(result.output).toContain('test4-done');
+  expect(result.output).toContain('test5-done');
 });
 
 test('should respect shard=2/2', async ({ runInlineTest }) => {
   const result = await runInlineTest(tests, { shard: '2/2' });
   expect(result.exitCode).toBe(0);
-  expect(result.passed).toBe(1);
+  expect(result.passed).toBe(2);
-  expect(result.skipped).toBe(3);
+  expect(result.skipped).toBe(0);
-  expect(result.output).toContain('test4-done');
+  expect(result.output).toContain('test1-done');
+  expect(result.output).toContain('test3-done');
 });
 
 test('should respect shard=1/2 in config', async ({ runInlineTest }) => {
@@ -64,8 +72,8 @@ test('should respect shard=1/2 in config', async ({ runInlineTest }) => {
   }, { shard: '1/2' });
   expect(result.exitCode).toBe(0);
   expect(result.passed).toBe(3);
-  expect(result.skipped).toBe(1);
+  expect(result.skipped).toBe(0);
-  expect(result.output).toContain('test1-done');
   expect(result.output).toContain('test2-done');
-  expect(result.output).toContain('test3-done');
+  expect(result.output).toContain('test4-done');
+  expect(result.output).toContain('test5-done');
 });