chore(runner): add all setup files in separate pass (#18978)

packages/playwright-test/src/runner.ts

@@ -235,13 +235,13 @@ export class Runner {
     return projects;
   }
 
-  private async _collectFiles(projects: FullProjectInternal[], commandLineFileFilters: TestFileFilter[]): Promise<{filesByProject: Map<FullProjectInternal, string[]>; setupFiles: Set<string>, applyFilterToSetup: boolean}> {
+  private async _collectFiles(projects: FullProjectInternal[], commandLineFileFilters: TestFileFilter[]): Promise<{filesByProject: Map<FullProjectInternal, string[]>; setupFiles: Set<string>}> {
     const extensions = ['.js', '.ts', '.mjs', '.tsx', '.jsx'];
     const testFileExtension = (file: string) => extensions.includes(path.extname(file));
     const filesByProject = new Map<FullProjectInternal, string[]>();
     const setupFiles = new Set<string>();
     const fileToProjectName = new Map<string, string>();
-    const commandLineFileMatcher = fileMatcherFrom(commandLineFileFilters);
+    const commandLineFileMatcher = commandLineFileFilters.length ? createFileMatcherFromFilters(commandLineFileFilters) : () => true;
     for (const project of projects) {
       const allFiles = await collectFiles(project.testDir, project._respectGitIgnore);
       const setupMatch = createFileMatcher(project._setup);
@@ -272,31 +272,44 @@ export class Runner {
       filesByProject.set(project, testFiles);
     }
 
-    // If none of the setup files matched the filter, we inlude all of them, otherwise
-    // only those that match the filter.
-    const applyFilterToSetup = !!commandLineFileFilters.length && [...setupFiles].some(commandLineFileMatcher);
-    if (applyFilterToSetup) {
-      for (const [project, files] of filesByProject) {
-        const filteredFiles = files.filter(commandLineFileMatcher);
-        if (filteredFiles.length)
-          filesByProject.set(project, filteredFiles);
-        else
-          filesByProject.delete(project);
-      }
-      for (const file of setupFiles) {
-        if (!commandLineFileMatcher(file))
-          setupFiles.delete(file);
-      }
-    }
-
-    return { filesByProject, setupFiles, applyFilterToSetup };
+    return { filesByProject, setupFiles };
   }
 
   private async _collectTestGroups(options: RunOptions, fatalErrors: TestError[]): Promise<{ rootSuite: Suite, projectSetupGroups: TestGroup[], testGroups: TestGroup[] }> {
     const config = this._loader.fullConfig();
     const projects = this._collectProjects(options.projectFilter);
-    const { filesByProject, setupFiles, applyFilterToSetup } = await this._collectFiles(projects, options.testFileFilters);
+    const { filesByProject, setupFiles } = await this._collectFiles(projects, options.testFileFilters);
 
+    let result = await this._createFilteredRootSuite(options, filesByProject, new Set(), !!setupFiles.size);
+    if (setupFiles.size) {
+      const allTests = result.rootSuite.allTests();
+      const tests = allTests.filter(test => !setupFiles.has(test._requireFile));
+      // If >0 tests match and
+      // - none of the setup files match the filter then we run all setup files,
+      // - if the filter also matches some of the setup tests, we'll run only
+      //   that maching subset of setup tests.
+      if (tests.length > 0 && tests.length === allTests.length)
+        result = await this._createFilteredRootSuite(options, filesByProject, setupFiles, false);
+    }
+
+    fatalErrors.push(...result.fatalErrors);
+    const { rootSuite } = result;
+
+    const allTestGroups = createTestGroups(rootSuite.suites, config.workers);
+    const projectSetupGroups = [];
+    const testGroups = [];
+    for (const group of allTestGroups) {
+      if (setupFiles.has(group.requireFile))
+        projectSetupGroups.push(group);
+      else
+        testGroups.push(group);
+    }
+    return { rootSuite, projectSetupGroups, testGroups };
+  }
+
+  private async _createFilteredRootSuite(options: RunOptions, filesByProject: Map<FullProjectInternal, string[]>, doNotFilterFiles: Set<string>, shouldCloneTests: boolean): Promise<{rootSuite: Suite, fatalErrors: TestError[]}> {
+    const config = this._loader.fullConfig();
+    const fatalErrors: TestError[] = [];
     const allTestFiles = new Set<string>();
     for (const files of filesByProject.values())
       files.forEach(file => allTestFiles.add(file));
@@ -307,7 +320,8 @@ export class Runner {
       const fileSuite = await this._loader.loadTestFile(file, 'runner');
       if (fileSuite._loadError)
         fatalErrors.push(fileSuite._loadError);
-      preprocessRoot._addSuite(fileSuite);
+      // We have to clone only if there maybe subsequent calls of this method.
+      preprocessRoot._addSuite(shouldCloneTests ? fileSuite._deepClone() : fileSuite);
     }
 
     // Complain about duplicate titles.
@@ -316,8 +330,7 @@ export class Runner {
       fatalErrors.push(duplicateTitlesError);
 
     // Filter tests to respect line/column filter.
-    if (options.testFileFilters.length)
-      filterByFocusedLine(preprocessRoot, options.testFileFilters, applyFilterToSetup ? new Set() : setupFiles);
+    filterByFocusedLine(preprocessRoot, options.testFileFilters, doNotFilterFiles);
 
     // Complain about only.
     if (config.forbidOnly) {
@@ -327,13 +340,8 @@ export class Runner {
     }
 
     // Filter only.
-    if (!options.listOnly) {
-      const onlyItems = preprocessRoot._getOnlyItems();
-      if (onlyItems.length) {
-        const hasOnlyInSetup = onlyItems.some(item => setupFiles.has(item._requireFile));
-        filterOnly(preprocessRoot, hasOnlyInSetup ? new Set() : setupFiles);
-      }
-    }
+    if (!options.listOnly)
+      filterOnly(preprocessRoot, doNotFilterFiles);
 
     // Generate projects.
     const fileSuites = new Map<string, Suite>();
@@ -346,6 +354,8 @@ export class Runner {
       const grepInvertMatcher = project.grepInvert ? createTitleMatcher(project.grepInvert) : null;
 
       const titleMatcher = (test: TestCase) => {
+        if (doNotFilterFiles.has(test._requireFile))
+          return true;
         const grepTitle = test.titlePath().join(' ');
         if (grepInvertMatcher?.(grepTitle))
           return false;
@@ -362,50 +372,13 @@ export class Runner {
         if (!fileSuite)
           continue;
         for (let repeatEachIndex = 0; repeatEachIndex < project.repeatEach; repeatEachIndex++) {
-          const builtSuite = this._loader.buildFileSuiteForProject(project, fileSuite, repeatEachIndex, test => {
-            if (setupFiles.has(test._requireFile))
-              return true;
-            return titleMatcher(test);
-          });
+          const builtSuite = this._loader.buildFileSuiteForProject(project, fileSuite, repeatEachIndex, titleMatcher);
           if (builtSuite)
             projectSuite._addSuite(builtSuite);
         }
       }
-
-      // At this point projectSuite contains all setup tests (unfiltered) and all regular
-      // tests matching the filter.
-      if (projectSuite.allTests().some(test => !setupFiles.has(test._requireFile))) {
-        // If >0 tests match and
-        // - none of the setup files match the filter then we run all setup files,
-        // - if the filter also matches some of the setup tests, we'll run only
-        //   that maching subset of setup tests.
-        const filterMatchesSetup = projectSuite.allTests().some(test => {
-          if (!setupFiles.has(test._requireFile))
-            return false;
-          return titleMatcher(test);
-        });
-        if (filterMatchesSetup) {
-          filterSuiteWithOnlySemantics(projectSuite, () => false, test => {
-            if (!setupFiles.has(test._requireFile))
-              return true;
-            return titleMatcher(test);
-          });
-        }
-      }
     }
-
-    const allTestGroups = createTestGroups(rootSuite.suites, config.workers);
-
-    const projectSetupGroups = [];
-    const testGroups = [];
-    for (const group of allTestGroups) {
-      if (setupFiles.has(group.requireFile))
-        projectSetupGroups.push(group);
-      else
-        testGroups.push(group);
-    }
-
-    return { rootSuite, projectSetupGroups, testGroups };
+    return { rootSuite, fatalErrors };
   }
 
   private _filterForCurrentShard(rootSuite: Suite, projectSetupGroups: TestGroup[], testGroups: TestGroup[]) {
@@ -677,26 +650,25 @@ export class Runner {
 }
 
 function filterOnly(suite: Suite, doNotFilterFiles: Set<string>) {
-  const suiteFilter = (suite: Suite) => suite._only;
-  const testFilter = (test: TestCase) => doNotFilterFiles.has(test._requireFile) || test._only;
+  if (!suite._getOnlyItems().length)
+    return;
+  const suiteFilter = (suite: Suite) => suite._only || doNotFilterFiles.has(suite._requireFile);
+  const testFilter = (test: TestCase) => test._only || doNotFilterFiles.has(test._requireFile);
   return filterSuiteWithOnlySemantics(suite, suiteFilter, testFilter);
 }
 
-function filterByFocusedLine(suite: Suite, focusedTestFileLines: TestFileFilter[], doNotFilterFiles: Set<string>) {
-  const filterWithLine = !!focusedTestFileLines.find(f => f.line !== null);
-  if (!filterWithLine)
-    return;
+function createFileMatcherFromFilter(filter: TestFileFilter) {
+  const fileMatcher = createFileMatcher(filter.re || filter.exact || '');
+  return (testFileName: string, testLine: number, testColumn: number) =>
+    fileMatcher(testFileName) && (filter.line === testLine || filter.line === null) && (filter.column === testColumn || filter.column === null);
+}
 
-  const testFileLineMatches = (testFileName: string, testLine: number, testColumn: number) => focusedTestFileLines.some(filter => {
-    const lineColumnOk = (filter.line === testLine || filter.line === null) && (filter.column === testColumn || filter.column === null);
-    if (!lineColumnOk)
-      return false;
-    return createFileMatcherFromFilters([filter])(testFileName);
-  });
-  const suiteFilter = (suite: Suite) => {
-    return !!suite.location && testFileLineMatches(suite.location.file, suite.location.line, suite.location.column);
-  };
-  // Project setup files are always included.
+function filterByFocusedLine(suite: Suite, focusedTestFileLines: TestFileFilter[], doNotFilterFiles: Set<string>) {
+  if (!focusedTestFileLines.length)
+    return;
+  const matchers = focusedTestFileLines.map(createFileMatcherFromFilter);
+  const testFileLineMatches = (testFileName: string, testLine: number, testColumn: number) => matchers.some(m => m(testFileName, testLine, testColumn));
+  const suiteFilter = (suite: Suite) => doNotFilterFiles.has(suite._requireFile) || !!suite.location && testFileLineMatches(suite.location.file, suite.location.line, suite.location.column);
   const testFilter = (test: TestCase) => doNotFilterFiles.has(test._requireFile) || testFileLineMatches(test.location.file, test.location.line, test.location.column);
   return filterSuite(suite, suiteFilter, testFilter);
 }
@@ -946,12 +918,6 @@ class ListModeReporter implements Reporter {
   }
 }
 
-function fileMatcherFrom(testFileFilters?: TestFileFilter[]): Matcher {
-  if (testFileFilters?.length)
-    return createFileMatcherFromFilters(testFileFilters);
-  return () => true;
-}
-
 function createForbidOnlyError(config: FullConfigInternal, onlyTestsAndSuites: (TestCase | Suite)[]): TestError {
   const errorMessage = [
     '=====================================',

packages/playwright-test/src/test.ts

@@ -104,6 +104,17 @@ export class Suite extends Base implements reporterTypes.Suite {
     return items;
   }
 
+  _deepClone(): Suite {
+    const suite = this._clone();
+    for (const entry of this._entries) {
+      if (entry instanceof Suite)
+        suite._addSuite(entry._deepClone());
+      else
+        suite._addTest(entry._clone());
+    }
+    return suite;
+  }
+
   _clone(): Suite {
     const suite = new Suite(this.title, this._type);
     suite._only = this._only;

tests/playwright-test/project-setup.spec.ts

@@ -498,6 +498,42 @@ test('should allow describe.only in setup files', async ({ runGroups }, testInfo
   expect(passed).toBe(2);
 });
 
+test('should filter describe line in setup files', async ({ runGroups }, testInfo) => {
+  const files = {
+    'playwright.config.ts': `
+      module.exports = {
+        projects: [
+          {
+            name: 'p1',
+            setup: /.*.setup.ts/,
+          },
+        ]
+      };`,
+    'a.test.ts': `
+      const { test } = pwt;
+      test('test1', async () => { });
+      test('test2', async () => { });
+      test('test3', async () => { });
+      test('test4', async () => { });
+    `,
+    'a.setup.ts': `
+      const { test } = pwt;
+      test.describe('main', () => {
+        test('setup1', async () => { });
+        test('setup2', async () => { });
+      });
+      test('setup3', async () => { });
+    `,
+  };
+
+  const { exitCode, passed, timeline, output } =  await runGroups(files, undefined, undefined, { additionalArgs: ['a.setup.ts:5'] });
+  expect(output).toContain('Running 2 tests using 1 worker');
+  expect(output).toContain('[p1] › a.setup.ts:6:9 › main › setup1');
+  expect(output).toContain('[p1] › a.setup.ts:7:9 › main › setup2');
+  expect(fileNames(timeline)).toEqual(['a.setup.ts']);
+  expect(exitCode).toBe(0);
+  expect(passed).toBe(2);
+});
 
 test('should allow .only in both setup and test files', async ({ runGroups }, testInfo) => {
   const files = {
@@ -785,5 +821,40 @@ test('should run all setup files if only tests match grep filter', async ({ runG
   expect(output).toContain('[p1] › a.test.ts:7:7 › test2');
 });
 
+test('should apply project.grep filter to both setup and tests', async ({ runGroups }, testInfo) => {
+  const files = {
+    'playwright.config.ts': `
+      module.exports = {
+        projects: [
+          {
+            name: 'p1',
+            setup: /.*.setup.ts/,
+            grep: /a.(test|setup).ts.*(test|setup)/,
+          },
+        ]
+      };`,
+    'a.test.ts': `
+      const { test } = pwt;
+      test('test1', async () => { });
+      test('test2', async () => { });
+      test('foo', async () => { });
+    `,
+    'a.setup.ts': `
+      const { test } = pwt;
+      test('setup1', async () => { });
+      test('setup2', async () => { });
+    `,
+    'b.setup.ts': `
+      const { test } = pwt;
+      test('setup1', async () => { });
+      test('foo', async () => { });
+    `,
+  };
 
-// TODO: test that grep applies to both setup and tests
+  const { exitCode, output } =  await runGroups(files);
+  expect(exitCode).toBe(0);
+  expect(output).toContain('[p1] › a.setup.ts:5:7 › setup1');
+  expect(output).toContain('[p1] › a.setup.ts:6:7 › setup2');
+  expect(output).toContain('[p1] › a.test.ts:6:7 › test1');
+  expect(output).toContain('[p1] › a.test.ts:7:7 › test2');
+});

tests/playwright-test/runner.spec.ts

@@ -479,3 +479,22 @@ test('should not crash with duplicate titles and line filter', async ({ runInlin
     `   - example.spec.ts:8`,
   ].join('\n'));
 });
+
+test('should not load tests not matching filter', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      const { test } = pwt;
+      console.log('in a.spec.ts');
+      test('test1', () => {});
+    `,
+    'example.spec.ts': `
+      const { test } = pwt;
+      console.log('in example.spec.ts');
+      test('test2', () => {});
+  `
+
+  }, {}, {}, { additionalArgs: ['a.spec.ts'] });
+  expect(result.exitCode).toBe(0);
+  expect(stripAnsi(result.output)).not.toContain('in example.spec.ts');
+  expect(stripAnsi(result.output)).toContain('in a.spec.ts');
+});