From 76e63c4bc6c958cee5f17c146fd7663196090551 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Thu, 18 Jul 2024 16:24:08 +0200 Subject: [PATCH] refactor --- packages/playwright/src/runner/loadUtils.ts | 54 ++++++++++++++++++--- packages/playwright/src/runner/tasks.ts | 5 +- packages/playwright/src/util.ts | 45 ++--------------- tests/playwright-test/only-changed.spec.ts | 2 +- 4 files changed, 54 insertions(+), 52 deletions(-) diff --git a/packages/playwright/src/runner/loadUtils.ts b/packages/playwright/src/runner/loadUtils.ts index fe0088cac0..99d832cec6 100644 --- a/packages/playwright/src/runner/loadUtils.ts +++ b/packages/playwright/src/runner/loadUtils.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import childProcess from 'child_process'; import path from 'path'; import type { FullConfig, Reporter, TestError } from '../../types/testReporter'; import { InProcessLoaderHost, OutOfProcessLoaderHost } from './loaderHost'; @@ -28,7 +29,7 @@ import type { TestRun } from './tasks'; import { requireOrImport } from '../transform/transform'; import { applyRepeatEachIndex, bindFileSuiteToProject, filterByFocusedLine, filterByTestIds, filterOnly, filterTestsRemoveEmptySuites } from '../common/suiteUtils'; import { createTestGroups, filterForShard, type TestGroup } from './testGroups'; -import { dependenciesForTestFile } from '../transform/compilationCache'; +import { affectedTestFiles, dependenciesForTestFile } from '../transform/compilationCache'; import { sourceMapSupport } from '../utilsBundle'; import type { RawSourceMap } from 'source-map'; @@ -36,7 +37,7 @@ export async function collectProjectsAndTestFiles(testRun: TestRun, doNotRunTest const config = testRun.config; const fsCache = new Map(); const sourceMapCache = new Map(); - const cliFileMatcher = config.cliArgs.length ? await createFileMatcherFromArguments(config.cliArgs, undefined) : null; + const cliFileMatcher = config.cliArgs.length ? createFileMatcherFromArguments(config.cliArgs) : null; // First collect all files for the projects in the command line, don't apply any file filters. const allFilesForProject = new Map(); @@ -118,7 +119,43 @@ export async function loadFileSuites(testRun: TestRun, mode: 'out-of-process' | } } -export async function createRootSuite(testRun: TestRun, errors: TestError[], shouldFilterOnly: boolean): Promise { +export async function detectChangedFiles(baseCommit: string): Promise { + function gitFileList(command: string) { + try { + return childProcess.execSync( + `git ${command}`, + { encoding: 'utf-8', stdio: 'pipe' } + ).split('\n').filter(Boolean); + } catch (_error) { + const error = _error as childProcess.SpawnSyncReturns; + throw new Error([ + `Encountered error while detecting changed files.`, + `--only-changed only works with Git repositories.`, + `Make sure that:`, + ` - You are running the test in a Git repository.`, + ` - The Git binary is in your PATH.`, + ` - The passed Git Ref exists in the repository. You passed '${baseCommit}'.`, + ``, + `Command Output:`, + ``, + ...error.output, + ].join('\n')); + } + } + + const untrackedFiles = gitFileList(`ls-files --others --exclude-standard`).map(file => path.join(process.cwd(), file)); + + const [gitRoot] = gitFileList('rev-parse --show-toplevel'); + const trackedFilesWithChanges = gitFileList(`diff ${baseCommit} --name-only`).map(file => path.join(gitRoot, file)); + + const filesWithChanges = [...untrackedFiles, ...trackedFilesWithChanges]; + return [ + ...filesWithChanges, + ...affectedTestFiles(filesWithChanges), + ]; +} + +export async function createRootSuite(testRun: TestRun, errors: TestError[], shouldFilterOnly: boolean, onlyChangedFiles?: string[]): Promise { const config = testRun.config; // Create root suite, where each child will be a project suite with cloned file suites inside it. const rootSuite = new Suite('', 'root'); @@ -128,7 +165,8 @@ export async function createRootSuite(testRun: TestRun, errors: TestError[], sho // Filter all the projects using grep, testId, file names. { // Interpret cli parameters. - const cliFileFilters = await createFileFiltersFromArguments(config.cliArgs, config.cliOnlyChanged); + const cliFileFilters = createFileFiltersFromArguments(config.cliArgs); + const onlyChangedFilters = onlyChangedFiles ? createFileFiltersFromArguments(onlyChangedFiles) : undefined; const grepMatcher = config.cliGrep ? createTitleMatcher(forceRegExp(config.cliGrep)) : () => true; const grepInvertMatcher = config.cliGrepInvert ? createTitleMatcher(forceRegExp(config.cliGrepInvert)) : () => false; const cliTitleMatcher = (title: string) => !grepInvertMatcher(title) && grepMatcher(title); @@ -137,7 +175,7 @@ export async function createRootSuite(testRun: TestRun, errors: TestError[], sho for (const [project, fileSuites] of testRun.projectSuites) { const projectSuite = createProjectSuite(project, fileSuites); projectSuites.set(project, projectSuite); - const filteredProjectSuite = filterProjectSuite(projectSuite, { cliFileFilters, cliTitleMatcher, testIdMatcher: config.testIdMatcher }); + const filteredProjectSuite = filterProjectSuite(projectSuite, { cliFileFilters, cliTitleMatcher, testIdMatcher: config.testIdMatcher, onlyChangedFilters }); filteredProjectSuites.set(project, filteredProjectSuite); } } @@ -223,14 +261,16 @@ function createProjectSuite(project: FullProjectInternal, fileSuites: Suite[]): return projectSuite; } -function filterProjectSuite(projectSuite: Suite, options: { cliFileFilters: TestFileFilter[], cliTitleMatcher?: Matcher, testIdMatcher?: Matcher }): Suite { +function filterProjectSuite(projectSuite: Suite, options: { cliFileFilters: TestFileFilter[], cliTitleMatcher?: Matcher, testIdMatcher?: Matcher, onlyChangedFilters?: TestFileFilter[] }): Suite { // Fast path. - if (!options.cliFileFilters.length && !options.cliTitleMatcher && !options.testIdMatcher) + if (!options.cliFileFilters.length && !options.cliTitleMatcher && !options.testIdMatcher && !options.onlyChangedFilters?.length) return projectSuite; const result = projectSuite._deepClone(); if (options.cliFileFilters.length) filterByFocusedLine(result, options.cliFileFilters); + if (options.onlyChangedFilters?.length) + filterByFocusedLine(result, options.onlyChangedFilters); if (options.testIdMatcher) filterByTestIds(result, options.testIdMatcher); filterTestsRemoveEmptySuites(result, (test: TestCase) => { diff --git a/packages/playwright/src/runner/tasks.ts b/packages/playwright/src/runner/tasks.ts index 5a69b94e8b..f9f7ba60a1 100644 --- a/packages/playwright/src/runner/tasks.ts +++ b/packages/playwright/src/runner/tasks.ts @@ -26,7 +26,7 @@ import { createTestGroups, type TestGroup } from '../runner/testGroups'; import type { Task } from './taskRunner'; import { TaskRunner } from './taskRunner'; import type { FullConfigInternal, FullProjectInternal } from '../common/config'; -import { collectProjectsAndTestFiles, createRootSuite, loadFileSuites, loadGlobalHook } from './loadUtils'; +import { collectProjectsAndTestFiles, createRootSuite, detectChangedFiles, loadFileSuites, loadGlobalHook } from './loadUtils'; import type { Matcher } from '../util'; import { Suite } from '../common/test'; import { buildDependentProjects, buildTeardownToSetupsMap, filterProjects } from './projectUtils'; @@ -228,7 +228,8 @@ function createLoadTask(mode: 'out-of-process' | 'in-process', options: { filter setup: async (testRun, errors, softErrors) => { await collectProjectsAndTestFiles(testRun, !!options.doNotRunDepsOutsideProjectFilter, options.additionalFileMatcher); await loadFileSuites(testRun, mode, options.failOnLoadErrors ? errors : softErrors); - testRun.rootSuite = await createRootSuite(testRun, options.failOnLoadErrors ? errors : softErrors, !!options.filterOnly); + const changedFiles = testRun.config.cliOnlyChanged ? await detectChangedFiles(testRun.config.cliOnlyChanged) : undefined; + testRun.rootSuite = await createRootSuite(testRun, options.failOnLoadErrors ? errors : softErrors, !!options.filterOnly, changedFiles); testRun.failureTracker.onRootSuite(testRun.rootSuite); // Fail when no tests. if (options.failOnLoadErrors && !testRun.rootSuite.allTests().length && !testRun.config.cliPassWithNoTests && !testRun.config.config.shard) { diff --git a/packages/playwright/src/util.ts b/packages/playwright/src/util.ts index bc58bd9dd6..8963f5cbb2 100644 --- a/packages/playwright/src/util.ts +++ b/packages/playwright/src/util.ts @@ -81,46 +81,7 @@ export type TestFileFilter = { column: number | null; }; -export async function detectChangedFiles(baseCommit: string): Promise { - function gitFileList(command: string) { - try { - return childProcess.execSync( - `git ${command}`, - { encoding: 'utf-8', stdio: 'pipe' } - ).split('\n').filter(Boolean); - } catch (_error) { - const error = _error as childProcess.SpawnSyncReturns; - throw new Error([ - `Encountered error while detecting changed files.`, - `--only-changed only works with Git repositories.`, - `Make sure that:`, - ` - You are running the test in a Git repository.`, - ` - The Git binary is in your PATH.`, - ` - The passed Git Ref exists in the repository. You passed '${baseCommit}'.`, - ``, - `Command Output:`, - ``, - ...error.output, - ].join('\n')); - } - } - - const untrackedFiles = gitFileList(`ls-files --others --exclude-standard`).map(file => path.join(process.cwd(), file)); - - const [gitRoot] = gitFileList('rev-parse --show-toplevel'); - const trackedFilesWithChanges = gitFileList(`diff ${baseCommit} --name-only`).map(file => path.join(gitRoot, file)); - - const filesWithChanges = [...untrackedFiles, ...trackedFilesWithChanges]; - return [ - ...filesWithChanges, - ...affectedTestFiles(filesWithChanges), - ]; -} - -export async function createFileFiltersFromArguments(args: string[], onlyChanged: string | undefined): Promise { - if (onlyChanged) - args = await detectChangedFiles(onlyChanged); - +export function createFileFiltersFromArguments(args: string[]): TestFileFilter[] { return args.map(arg => { const match = /^(.*?):(\d+):?(\d+)?$/.exec(arg); return { @@ -131,8 +92,8 @@ export async function createFileFiltersFromArguments(args: string[], onlyChanged }); } -export async function createFileMatcherFromArguments(args: string[], onlyChanged: string | undefined): Promise { - const filters = await createFileFiltersFromArguments(args, onlyChanged); +export function createFileMatcherFromArguments(args: string[]): Matcher { + const filters = createFileFiltersFromArguments(args); return createFileMatcher(filters.map(filter => filter.re || filter.exact || '')); } diff --git a/tests/playwright-test/only-changed.spec.ts b/tests/playwright-test/only-changed.spec.ts index 03546f4c16..0cbbe6d878 100644 --- a/tests/playwright-test/only-changed.spec.ts +++ b/tests/playwright-test/only-changed.spec.ts @@ -144,7 +144,7 @@ test('should throw nice error message if git doesnt work', async ({ setupReposit expect(result.output).toContain('only works with Git repositories'); }); -test.only('should suppport component tests', async ({ runInlineTest, setupRepository, writeFiles }) => { +test.skip('should suppport component tests', async ({ runInlineTest, setupRepository, writeFiles }) => { const git = await setupRepository(); await writeFiles({