fix(test runner): properly keep track of requireFile to support helpers/wrappers (#7243)

This fixes an issue where we incorrectly labeled and assigned ids for tests
that declared tests in require'd files or used test wrappers.
See new tests for examples.

src/test/dispatcher.ts

@@ -82,7 +82,7 @@ export class Dispatcher {
   _filesSortedByWorkerHash(): DispatcherEntry[] {
     const entriesByWorkerHashAndFile = new Map<string, Map<string, DispatcherEntry>>();
     for (const fileSuite of this._suite.suites) {
-      const file = fileSuite.file;
+      const file = fileSuite._requireFile;
       for (const spec of fileSuite._allSpecs()) {
         for (const test of spec.tests) {
           let entriesByFile = entriesByWorkerHashAndFile.get(test._workerHash);

src/test/loader.ts

@@ -117,6 +117,7 @@ export class Loader {
     const revertBabelRequire = installTransform();
     try {
       const suite = new Suite('');
+      suite._requireFile = file;
       suite.file = file;
       setCurrentlyLoadingFileSuite(suite);
       require(file);

src/test/project.ts

@@ -90,7 +90,7 @@ export class ProjectImpl {
       test._repeatEachIndex = i;
       test._projectIndex = this.index;
       test._workerHash = `run${this.index}-${digest}-repeat${i}`;
-      test._id = `${spec._ordinalInFile}@${spec.file}#run${this.index}-repeat${i}`;
+      test._id = `${spec._ordinalInFile}@${spec._requireFile}#run${this.index}-repeat${i}`;
       spec.tests.push(test);
       tests.push(test);
     }

src/test/runner.ts

@@ -165,7 +165,7 @@ export class Runner {
 
       const fileSuites = new Map<string, Suite>();
       for (const fileSuite of rootSuite.suites)
-        fileSuites.set(fileSuite.file, fileSuite);
+        fileSuites.set(fileSuite._requireFile, fileSuite);
 
       const outputDirs = new Set<string>();
       const grepMatcher = createMatcher(config.grep);
@@ -209,7 +209,7 @@ export class Runner {
       if (process.stdout.isTTY) {
         const workers = new Set();
         rootSuite.findTest(test => {
-          workers.add(test.spec.file + test._workerHash);
+          workers.add(test.spec._requireFile + test._workerHash);
         });
         console.log();
         const jobs = Math.min(config.workers, workers.size);

src/test/test.ts

@@ -26,6 +26,7 @@ class Base {
   parent?: Suite;
 
   _only = false;
+  _requireFile: string = '';
 
   constructor(title: string) {
     this.title = title;

src/test/testType.ts

@@ -63,10 +63,11 @@ export class TestTypeImpl {
       throw errorWithCallLocation(`test() can only be called in a test file`);
     const location = callLocation(suite.file);
 
-    const ordinalInFile = countByFile.get(suite.file) || 0;
+    const ordinalInFile = countByFile.get(suite._requireFile) || 0;
-    countByFile.set(location.file, ordinalInFile + 1);
+    countByFile.set(suite._requireFile, ordinalInFile + 1);
 
     const spec = new Spec(title, fn, ordinalInFile, this);
+    spec._requireFile = suite._requireFile;
     spec.file = location.file;
     spec.line = location.line;
     spec.column = location.column;
@@ -83,7 +84,8 @@ export class TestTypeImpl {
     const location = callLocation(suite.file);
 
     const child = new Suite(title);
-    child.file = suite.file;
+    child._requireFile = suite._requireFile;
+    child.file = location.file;
     child.line = location.line;
     child.column = location.column;
     suite._addSuite(child);

src/test/workerRunner.ts

@@ -184,7 +184,7 @@ export class WorkerRunner extends EventEmitter {
     const testId = test._id;
 
     const baseOutputDir = (() => {
-      const relativeTestFilePath = path.relative(this._project.config.testDir, spec.file.replace(/\.(spec|test)\.(js|ts)/, ''));
+      const relativeTestFilePath = path.relative(this._project.config.testDir, spec._requireFile.replace(/\.(spec|test)\.(js|ts)/, ''));
       const sanitizedRelativePath = relativeTestFilePath.replace(process.platform === 'win32' ? new RegExp('\\\\', 'g') : new RegExp('/', 'g'), '-');
       let testOutputDir = sanitizedRelativePath + '-' + sanitizeForFilePath(spec.title);
       if (this._uniqueProjectNamePathSegment)
@@ -231,7 +231,7 @@ export class WorkerRunner extends EventEmitter {
           else
             snapshotName += suffix;
         }
-        return path.join(spec.file + '-snapshots', snapshotName);
+        return path.join(spec._requireFile + '-snapshots', snapshotName);
       },
       skip: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'skip', args),
       fixme: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'fixme', args),

tests/playwright-test/basic.spec.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { test, expect } from './playwright-test-fixtures';
+import { test, expect, stripAscii } from './playwright-test-fixtures';
 import * as path from 'path';
 
 test('should fail', async ({ runInlineTest }) => {
@@ -210,7 +210,7 @@ test('skip should take priority over fail', async ({ runInlineTest }) => {
   expect(failed).toBe(1);
 });
 
-test('should focus test from one runTests', async ({ runInlineTest }) => {
+test('should focus test from one project', async ({ runInlineTest }) => {
   const { exitCode, passed, skipped, failed } = await runInlineTest({
     'playwright.config.ts': `
       import * as path from 'path';
@@ -251,3 +251,103 @@ test('should work with default export', async ({ runInlineTest }) => {
   expect(result.passed).toBe(1);
   expect(result.failed).toBe(0);
 });
+
+test('should work with test wrapper', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'helper.js': `
+      console.log('%%helper');
+      exports.wrap = (title, fn) => {
+        pwt.test(title, fn);
+      };
+    `,
+    'a.spec.js': `
+      console.log('%%a.spec');
+      const { wrap } = require('./helper');
+      wrap('test1', () => {
+        console.log('%%test1');
+      });
+      pwt.test.describe('suite1', () => {
+        wrap('suite1.test1', () => {
+          console.log('%%suite1.test1');
+        });
+      });
+    `,
+    'b.spec.js': `
+      console.log('%%b.spec');
+      const { wrap } = require('./helper');
+      wrap('test2', () => {
+        console.log('%%test2');
+      });
+      pwt.test.describe('suite2', () => {
+        wrap('suite2.test2', () => {
+          console.log('%%suite2.test2');
+        });
+      });
+    `,
+  }, { workers: 1, reporter: 'line' });
+  expect(result.passed).toBe(4);
+  expect(result.exitCode).toBe(0);
+  expect(stripAscii(result.output).split('\n').filter(line => line.startsWith('%%'))).toEqual([
+    '%%a.spec',
+    '%%helper',
+    '%%b.spec',
+    '%%a.spec',
+    '%%helper',
+    '%%test1',
+    '%%suite1.test1',
+    '%%b.spec',
+    '%%test2',
+    '%%suite2.test2',
+  ]);
+});
+
+test('should work with test helper', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'helper-a.js': `
+      console.log('%%helper-a');
+      pwt.test('test1', () => {
+        console.log('%%test1');
+      });
+      pwt.test.describe('suite1', () => {
+        pwt.test('suite1.test1', () => {
+          console.log('%%suite1.test1');
+        });
+      });
+    `,
+    'a.spec.js': `
+      console.log('%%a.spec');
+      require('./helper-a');
+    `,
+    'helper-b.js': `
+      console.log('%%helper-b');
+      pwt.test('test1', () => {
+        console.log('%%test2');
+      });
+      pwt.test.describe('suite2', () => {
+        pwt.test('suite2.test2', () => {
+          console.log('%%suite2.test2');
+        });
+      });
+    `,
+    'b.spec.js': `
+      console.log('%%b.spec');
+      require('./helper-b');
+    `,
+  }, { workers: 1, reporter: 'line' });
+  expect(result.passed).toBe(4);
+  expect(result.exitCode).toBe(0);
+  expect(stripAscii(result.output).split('\n').filter(line => line.startsWith('%%'))).toEqual([
+    '%%a.spec',
+    '%%helper-a',
+    '%%b.spec',
+    '%%helper-b',
+    '%%a.spec',
+    '%%helper-a',
+    '%%test1',
+    '%%suite1.test1',
+    '%%b.spec',
+    '%%helper-b',
+    '%%test2',
+    '%%suite2.test2',
+  ]);
+});