From 6ec936f808cccf661596f891bd8efb3ac7ffb631 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 22 Jul 2024 13:53:54 +0200 Subject: [PATCH] address most of the review --- docs/src/test-cli-js.md | 7 +- packages/playwright/src/plugins/index.ts | 2 +- packages/playwright/src/program.ts | 2 +- packages/playwright/src/runner/loadUtils.ts | 47 +--------- packages/playwright/src/runner/tasks.ts | 3 +- packages/playwright/src/runner/vcs.ts | 58 +++++++++++++ tests/playwright-test/only-changed.spec.ts | 95 +++++++++------------ 7 files changed, 106 insertions(+), 108 deletions(-) create mode 100644 packages/playwright/src/runner/vcs.ts diff --git a/docs/src/test-cli-js.md b/docs/src/test-cli-js.md index a48753033c..052c0abf1f 100644 --- a/docs/src/test-cli-js.md +++ b/docs/src/test-cli-js.md @@ -22,11 +22,6 @@ Here are the most common options available in the command line. npx playwright test tests/todo-page/ tests/landing-page/ ``` -- Run only test files that have local changes - ```bash - npx playwright test --only-changed - ``` - - Run files that have `my-spec` or `my-spec-2` in the file name ```bash npx playwright test my-spec my-spec-2 @@ -98,7 +93,7 @@ Complete set of Playwright Test options is available in the [configuration file] | `--max-failures ` or `-x`| Stop after the first `N` test failures. Passing `-x` stops after the first failure.| | `--no-deps` | Ignore the dependencies between projects and behave as if they were not specified. | | `--output ` | Directory for artifacts produced by tests, defaults to `test-results`. | -| `--only-changed [ref]` | Only run tests that have been changed between `HEAD` and `ref`. Defaults to running all uncommitted changes. Only supports Git. | +| `--only-changed [ref]` | Only run tests that have been changed between working tree and "ref". Defaults to running all uncommitted changes with ref=HEAD. Only supports Git. | | `--pass-with-no-tests` | Allows the test suite to pass when no files are found. | | `--project ` | Only run tests from the specified [projects](./test-projects.md), supports '*' wildcard. Defaults to running all projects defined in the configuration file.| | `--quiet` | Whether to suppress stdout and stderr from the tests. | diff --git a/packages/playwright/src/plugins/index.ts b/packages/playwright/src/plugins/index.ts index f9532e0839..3502d90966 100644 --- a/packages/playwright/src/plugins/index.ts +++ b/packages/playwright/src/plugins/index.ts @@ -20,10 +20,10 @@ import type { ReporterV2 } from '../reporters/reporterV2'; export interface TestRunnerPlugin { name: string; setup?(config: FullConfig, configDir: string, reporter: ReporterV2): Promise; + populateDependencies?(): Promise; begin?(suite: Suite): Promise; end?(): Promise; teardown?(): Promise; - populateDependencies?(): Promise; } export type TestRunnerPluginRegistration = { diff --git a/packages/playwright/src/program.ts b/packages/playwright/src/program.ts index b8a13c2cc2..7cbe7e5461 100644 --- a/packages/playwright/src/program.ts +++ b/packages/playwright/src/program.ts @@ -159,7 +159,7 @@ async function runTests(args: string[], opts: { [key: string]: any }) { if (opts.ui || opts.uiHost || opts.uiPort) { if (opts.onlyChanged) - throw new Error(`--only-changed is not supported in UI mode. If you'd like that to change, please upvote https://github.com/microsoft/playwright/pull/31727#issuecomment-2239073584.`); + throw new Error(`--only-changed is not supported in UI mode. If you'd like that to change, see https://github.com/microsoft/playwright/issues/15075 for more details.`); const status = await testServer.runUIMode(opts.config, { host: opts.uiHost, diff --git a/packages/playwright/src/runner/loadUtils.ts b/packages/playwright/src/runner/loadUtils.ts index dfbaa27bac..20c7117c38 100644 --- a/packages/playwright/src/runner/loadUtils.ts +++ b/packages/playwright/src/runner/loadUtils.ts @@ -14,7 +14,6 @@ * 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'; @@ -29,11 +28,10 @@ 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 { affectedTestFiles, dependenciesForTestFile } from '../transform/compilationCache'; +import { dependenciesForTestFile } from '../transform/compilationCache'; import { sourceMapSupport } from '../utilsBundle'; import type { RawSourceMap } from 'source-map'; -const toPosixPath = (s: string) => s.replaceAll(path.sep, path.posix.sep); export async function collectProjectsAndTestFiles(testRun: TestRun, doNotRunTestsOutsideProjectFilter: boolean, additionalFileMatcher?: Matcher) { const config = testRun.config; @@ -121,49 +119,6 @@ export async function loadFileSuites(testRun: TestRun, mode: 'out-of-process' | } } -export async function detectChangedFiles(testRun: TestRun): Promise { - const baseCommit = testRun.config.cliOnlyChanged; - - 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]; - - for (const plugin of testRun.config.plugins) - await plugin.instance?.populateDependencies?.(); - const affectedFiles = affectedTestFiles(filesWithChanges); - - return [ - ...filesWithChanges, - ...affectedFiles, - ].map(toPosixPath); -} - 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. diff --git a/packages/playwright/src/runner/tasks.ts b/packages/playwright/src/runner/tasks.ts index 54466f59df..c329fb579e 100644 --- a/packages/playwright/src/runner/tasks.ts +++ b/packages/playwright/src/runner/tasks.ts @@ -26,11 +26,12 @@ 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, detectChangedFiles, loadFileSuites, loadGlobalHook } from './loadUtils'; +import { collectProjectsAndTestFiles, createRootSuite, loadFileSuites, loadGlobalHook } from './loadUtils'; import type { Matcher } from '../util'; import { Suite } from '../common/test'; import { buildDependentProjects, buildTeardownToSetupsMap, filterProjects } from './projectUtils'; import { FailureTracker } from './failureTracker'; +import { detectChangedFiles } from './vcs'; const readDirAsync = promisify(fs.readdir); diff --git a/packages/playwright/src/runner/vcs.ts b/packages/playwright/src/runner/vcs.ts new file mode 100644 index 0000000000..2d943a70df --- /dev/null +++ b/packages/playwright/src/runner/vcs.ts @@ -0,0 +1,58 @@ +/** + * Copyright Microsoft Corporation. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import childProcess from 'child_process'; +import { toPosixPath } from 'playwright-core/lib/utils'; +import { affectedTestFiles } from '../transform/compilationCache'; +import type { TestRun } from './tasks'; +import path from 'path'; + +export async function detectChangedFiles(testRun: TestRun): Promise { + const baseCommit = testRun.config.cliOnlyChanged; + + 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([ + `Cannot detect changed files for --only-changed mode:`, + `git ${command}`, + '', + ...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]; + + for (const plugin of testRun.config.plugins) + await plugin.instance?.populateDependencies?.(); + const affectedFiles = affectedTestFiles(filesWithChanges); + + return [ + ...filesWithChanges, + ...affectedFiles, + ].map(toPosixPath); +} \ No newline at end of file diff --git a/tests/playwright-test/only-changed.spec.ts b/tests/playwright-test/only-changed.spec.ts index a948a107ea..51f1af5895 100644 --- a/tests/playwright-test/only-changed.spec.ts +++ b/tests/playwright-test/only-changed.spec.ts @@ -18,49 +18,47 @@ import { test as baseTest, expect, playwrightCtConfigText } from './playwright-t import { execSync } from 'node:child_process'; const test = baseTest.extend({ - setupRepository: async ({ writeFiles }, use, testInfo) => { + git: async ({ writeFiles }, use, testInfo) => { const baseDir = testInfo.outputPath(); const git = (command: string) => execSync(`git ${command}`, { cwd: baseDir }); - await use(async () => { - await writeFiles({ - 'a.spec.ts': ` - import { test, expect } from '@playwright/test'; - import { answer, question } from './utils'; - test('fails', () => { expect(question).toBe(answer); }); - `, - 'b.spec.ts': ` - import { test, expect } from '@playwright/test'; - import { answer, question } from './utils'; - test('fails', () => { expect(question).toBe(answer); }); - `, - 'utils.ts': ` - export * from './answer'; - export * from './question'; - `, - 'answer.ts': ` - export const answer = 42; - `, - 'question.ts': ` - export const question = "???"; - `, - }); - git(`init --initial-branch=main`); - git(`config --local user.name "Robert Botman"`); - git(`config --local user.email "botty@mcbotface.com"`); - git(`add .`); - git(`commit -m init`); - return git; + await writeFiles({ + 'a.spec.ts': ` + import { test, expect } from '@playwright/test'; + import { answer, question } from './utils'; + test('fails', () => { expect(question).toBe(answer); }); + `, + 'b.spec.ts': ` + import { test, expect } from '@playwright/test'; + import { answer, question } from './utils'; + test('fails', () => { expect(question).toBe(answer); }); + `, + 'utils.ts': ` + export * from './answer'; + export * from './question'; + `, + 'answer.ts': ` + export const answer = 42; + `, + 'question.ts': ` + export const question = "???"; + `, }); + + git(`init --initial-branch=main`); + git(`config --local user.name "Robert Botman"`); + git(`config --local user.email "botty@mcbotface.com"`); + git(`add .`); + git(`commit -m init`); + + await use((command: string) => git(command)); }, }); test.slow(); -test('should detect untracked files', async ({ runInlineTest, setupRepository }) => { - await setupRepository(); - +test('should detect untracked files', async ({ runInlineTest, git }) => { const result = await runInlineTest({ 'c.spec.ts': ` import { test, expect } from '@playwright/test'; @@ -74,8 +72,7 @@ test('should detect untracked files', async ({ runInlineTest, setupRepository }) }); -test('should detect changed files', async ({ runInlineTest, setupRepository }) => { - await setupRepository(); +test('should detect changed files', async ({ runInlineTest, git }) => { const result = await runInlineTest({ 'b.spec.ts': ` import { test, expect } from '@playwright/test'; @@ -88,8 +85,7 @@ test('should detect changed files', async ({ runInlineTest, setupRepository }) = expect(result.output).toContain('b.spec.ts'); }); -test('should diff based on base commit', async ({ runInlineTest, setupRepository, writeFiles }) => { - const git = await setupRepository(); +test('should diff based on base commit', async ({ runInlineTest, git, writeFiles }) => { await writeFiles({ 'b.spec.ts': ` import { test, expect } from '@playwright/test'; @@ -104,8 +100,7 @@ test('should diff based on base commit', async ({ runInlineTest, setupRepository expect(result.output).toContain('b.spec.ts'); }); -test('should understand dependency structure', async ({ runInlineTest, setupRepository, writeFiles }) => { - await setupRepository(); +test('should understand dependency structure', async ({ runInlineTest, git, writeFiles }) => { await writeFiles({ 'question.ts': ` export const question = "what is the answer to life the universe and everything"; @@ -119,8 +114,7 @@ test('should understand dependency structure', async ({ runInlineTest, setupRepo expect(result.output).toContain('b.spec.ts'); }); -test('should support watch mode', async ({ setupRepository, writeFiles, runWatchTest }) => { - const git = await setupRepository(); +test('should support watch mode', async ({ git, writeFiles, runWatchTest }) => { await writeFiles({ 'b.spec.ts': ` import { test, expect } from '@playwright/test'; @@ -138,17 +132,16 @@ test('should support watch mode', async ({ setupRepository, writeFiles, runWatch expect(testProcess.output).not.toContain('a.spec'); }); -test('should throw nice error message if git doesnt work', async ({ setupRepository, runInlineTest }) => { - await setupRepository(); +test('should throw nice error message if git doesnt work', async ({ git, runInlineTest }) => { const result = await runInlineTest({}, { 'only-changed': `this-commit-does-not-exist` }); expect(result.exitCode).toBe(1); - expect(result.output).toContain('only works with Git repositories'); + expect(result.output, 'contains our error message').toContain('Cannot detect changed files for --only-changed mode'); + expect(result.output, 'contains command').toContain('git diff this-commit-does-not-exist --name-only'); + expect(result.output, 'contains git command output').toContain('unknown revision or path not in the working tree'); }); -test('should suppport component tests', async ({ runInlineTest, setupRepository, writeFiles }) => { - const git = await setupRepository(); - +test('should suppport component tests', async ({ runInlineTest, git, writeFiles }) => { await writeFiles({ 'playwright.config.ts': playwrightCtConfigText, 'playwright/index.html': ``, @@ -220,9 +213,7 @@ test('should suppport component tests', async ({ runInlineTest, setupRepository, }); test.describe('should work the same if being called in subdirectory', () => { - test('tracked file', async ({ runInlineTest, setupRepository, writeFiles }) => { - const git = await setupRepository(); - + test('tracked file', async ({ runInlineTest, git, writeFiles }) => { await writeFiles({ 'tests/c.spec.ts': ` import { test, expect } from '@playwright/test'; @@ -244,9 +235,7 @@ test.describe('should work the same if being called in subdirectory', () => { expect(result.output).toContain('c.spec.ts'); }); - test('untracked file', async ({ runInlineTest, setupRepository }) => { - await setupRepository(); - + test('untracked file', async ({ runInlineTest, git }) => { const result = await runInlineTest({ 'tests/c.spec.ts': ` import { test, expect } from '@playwright/test';