From 6769a311ed05f079375e3b5d28e01d6c7bbf7efd Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 2 Mar 2023 15:09:50 -0800 Subject: [PATCH] feat(test runner): error out when one test file imports another (#21357) This situation is not supported, and we can now detect it by looking at collected file dependencies. Fixes #21270. --- .../src/common/compilationCache.ts | 4 +++ .../playwright-test/src/runner/loadUtils.ts | 26 +++++++++++++++---- tests/playwright-test/loader.spec.ts | 22 ++++++++++++++++ 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/packages/playwright-test/src/common/compilationCache.ts b/packages/playwright-test/src/common/compilationCache.ts index fd11c40a8d..898c8a53b0 100644 --- a/packages/playwright-test/src/common/compilationCache.ts +++ b/packages/playwright-test/src/common/compilationCache.ts @@ -173,6 +173,10 @@ export function collectAffectedTestFiles(dependency: string, testFileCollector: } } +export function dependenciesForTestFile(filename: string): Set { + return fileDependencies.get(filename) || new Set(); +} + // These two are only used in the dev mode, they are specifically excluding // files from packages/playwright*. In production mode, node_modules covers // that. diff --git a/packages/playwright-test/src/runner/loadUtils.ts b/packages/playwright-test/src/runner/loadUtils.ts index 995d0efcac..c29e6d19e0 100644 --- a/packages/playwright-test/src/runner/loadUtils.ts +++ b/packages/playwright-test/src/runner/loadUtils.ts @@ -26,6 +26,7 @@ import { buildProjectsClosure, collectFilesForProject, filterProjects } from './ import { requireOrImport } from '../common/transform'; import { buildFileSuiteForProject, filterByFocusedLine, filterByTestIds, filterOnly, filterTestsRemoveEmptySuites } from '../common/suiteUtils'; import { filterForShard } from './testGroups'; +import { dependenciesForTestFile } from '../common/compilationCache'; export async function loadAllTests(mode: 'out-of-process' | 'in-process', config: FullConfigInternal, projectsToIgnore: Set, fileMatcher: Matcher, errors: TestError[], shouldFilterOnly: boolean): Promise { const projects = filterProjects(config.projects, config._internal.cliProjectFilter); @@ -78,7 +79,7 @@ export async function loadAllTests(mode: 'out-of-process' | 'in-process', config } // Load all test files and create a preprocessed root. Child suites are files there. - const fileSuits: Suite[] = []; + const fileSuites: Suite[] = []; { const loaderHost = mode === 'out-of-process' ? new OutOfProcessLoaderHost(config) : new InProcessLoaderHost(config); const allTestFiles = new Set(); @@ -86,13 +87,28 @@ export async function loadAllTests(mode: 'out-of-process' | 'in-process', config files.forEach(file => allTestFiles.add(file)); for (const file of allTestFiles) { const fileSuite = await loaderHost.loadTestFile(file, errors); - fileSuits.push(fileSuite); + fileSuites.push(fileSuite); } await loaderHost.stop(); + + // Check that no test file imports another test file. + // Loader must be stopped first, since it popuplates the dependency tree. + for (const file of allTestFiles) { + for (const dependency of dependenciesForTestFile(file)) { + if (allTestFiles.has(dependency)) { + const importer = path.relative(config.rootDir, file); + const importee = path.relative(config.rootDir, dependency); + errors.push({ + message: `Error: test file "${importer}" should not import test file "${importee}"`, + location: { file, line: 1, column: 1 }, + }); + } + } + } } // Complain about duplicate titles. - errors.push(...createDuplicateTitlesErrors(config, fileSuits)); + errors.push(...createDuplicateTitlesErrors(config, fileSuites)); // Create root suites with clones for the projects. const rootSuite = new Suite('', 'root'); @@ -105,7 +121,7 @@ export async function loadAllTests(mode: 'out-of-process' | 'in-process', config // First iterate leaf projects to focus only, then add all other projects. for (const project of topLevelProjects) { - const projectSuite = await createProjectSuite(fileSuits, project, { cliFileFilters, cliTitleMatcher, testIdMatcher: config._internal.testIdMatcher }, filesToRunByProject.get(project)!); + const projectSuite = await createProjectSuite(fileSuites, project, { cliFileFilters, cliTitleMatcher, testIdMatcher: config._internal.testIdMatcher }, filesToRunByProject.get(project)!); if (projectSuite) rootSuite._addSuite(projectSuite); } @@ -123,7 +139,7 @@ export async function loadAllTests(mode: 'out-of-process' | 'in-process', config // Prepend the projects that are dependencies. for (const project of dependencyProjects) { - const projectSuite = await createProjectSuite(fileSuits, project, { cliFileFilters: [], cliTitleMatcher: undefined }, filesToRunByProject.get(project)!); + const projectSuite = await createProjectSuite(fileSuites, project, { cliFileFilters: [], cliTitleMatcher: undefined }, filesToRunByProject.get(project)!); if (projectSuite) rootSuite._prependSuite(projectSuite); } diff --git a/tests/playwright-test/loader.spec.ts b/tests/playwright-test/loader.spec.ts index 0c04e3988c..160b78c724 100644 --- a/tests/playwright-test/loader.spec.ts +++ b/tests/playwright-test/loader.spec.ts @@ -641,3 +641,25 @@ test('should support node imports', async ({ runInlineTest, nodeVersion }) => { expect(result.passed).toBe(1); expect(result.exitCode).toBe(0); }); + +test('should complain when one test file imports another', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + import { foo } from './b.test'; + + test('pass1', async () => { + expect(foo).toBe('foo'); + }); + `, + 'b.test.ts': ` + import { test, expect } from '@playwright/test'; + export const foo = 'foo'; + + test('pass2', async () => { + }); + `, + }); + expect(result.exitCode).toBe(1); + expect(result.output).toContain(`test file "a.test.ts" should not import test file "b.test.ts"`); +});