address most of the review

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 <N>` 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 <dir>` | 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 <name>` | 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. |

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<void>;
+  populateDependencies?(): Promise<void>;
   begin?(suite: Suite): Promise<void>;
   end?(): Promise<void>;
   teardown?(): Promise<void>;
-  populateDependencies?(): Promise<void>;
 }
 
 export type TestRunnerPluginRegistration = {

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,

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<string[]> {
-  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<string>;
-      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<Suite> {
   const config = testRun.config;
   // Create root suite, where each child will be a project suite with cloned file suites inside it.

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);
 

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<string[]> {
+  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<string>;
+      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);
+}

tests/playwright-test/only-changed.spec.ts

@@ -18,12 +18,11 @@ 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';
@@ -46,21 +45,20 @@ const test = baseTest.extend({
       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 use((command: string) => git(command));
   },
 });
 
 test.slow();
 
-test('should detect untracked files', async ({ runInlineTest, setupRepository }) => {
+test('should detect untracked files', async ({ runInlineTest, git }) => {
-  await setupRepository();
-
   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 }) => {
+test('should detect changed files', async ({ runInlineTest, git }) => {
-  await setupRepository();
   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 }) => {
+test('should diff based on base commit', async ({ runInlineTest, git, writeFiles }) => {
-  const git = await setupRepository();
   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 }) => {
+test('should understand dependency structure', async ({ runInlineTest, git, writeFiles }) => {
-  await setupRepository();
   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 }) => {
+test('should support watch mode', async ({ git, writeFiles, runWatchTest }) => {
-  const git = await setupRepository();
   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 }) => {
+test('should throw nice error message if git doesnt work', async ({ git, runInlineTest }) => {
-  await setupRepository();
   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 }) => {
+test('should suppport component tests', async ({ runInlineTest, git, writeFiles }) => {
-  const git = await setupRepository();
-
   await writeFiles({
     'playwright.config.ts': playwrightCtConfigText,
     'playwright/index.html': `<script type="module" src="./index.ts"></script>`,
@@ -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 }) => {
+  test('tracked file', async ({ runInlineTest, git, writeFiles }) => {
-    const git = await setupRepository();
-
     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 }) => {
+  test('untracked file', async ({ runInlineTest, git }) => {
-    await setupRepository();
-
     const result = await runInlineTest({
       'tests/c.spec.ts': `
         import { test, expect } from '@playwright/test';