cherry-pick(#35062): do not compute git diff on non! PRs

Author: pavelfeldman

docs/src/test-api/class-testconfig.md

@@ -39,12 +39,10 @@ export default defineConfig({
 ## property: TestConfig.captureGitInfo
 * since: v1.51
 - type: ?<[Object]>
-  - `commit` ?<boolean> Whether to capture commit information such as hash, author, timestamp.
+  - `commit` ?<boolean> Whether to capture commit and pull request information such as hash, author, timestamp.
   - `diff` ?<boolean> Whether to capture commit diff.
 
-* These settings control whether git information is captured and stored in the config [`property: TestConfig.metadata`].
-* The structure of the git commit metadata is subject to change.
-* Default values for these settings depend on the environment. When tests run as a part of CI where it is safe to obtain git information, the default value is true, false otherwise.
+These settings control whether git information is captured and stored in the config [`property: TestConfig.metadata`].
 
 **Usage**
 
@@ -56,6 +54,20 @@ export default defineConfig({
 });
 ```
 
+**Details**
+
+* Capturing `commit` information is useful when you'd like to see it in your HTML (or a third party) report.
+* Capturing `diff` information is useful to enrich the report with the actual source diff. This information can be used to provide intelligent advice on how to fix the test.
+
+:::note
+Default values for these settings depend on the environment. When tests run as a part of CI where it is safe to obtain git information, the default value is `true`, `false` otherwise.
+:::
+
+:::note
+The structure of the git commit metadata is subject to change.
+:::
+
+
 ## property: TestConfig.expect
 * since: v1.10
 - type: ?<[Object]>

packages/playwright/src/isomorphic/types.d.ts

@@ -36,9 +36,9 @@ export type CIInfo = {
   commitHref: string;
   prHref?: string;
   prTitle?: string;
+  prBaseHash?: string;
   buildHref?: string;
   commitHash?: string;
-  baseHash?: string;
   branch?: string;
 };
 

packages/playwright/src/plugins/gitCommitInfoPlugin.ts

@@ -71,22 +71,19 @@ async function ciInfo(): Promise<CIInfo | undefined> {
 
     return {
       commitHref: `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/commit/${process.env.GITHUB_SHA}`,
+      commitHash: process.env.GITHUB_SHA,
       prHref: pr ? `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/pull/${pr.number}` : undefined,
-      prTitle: pr ? pr.title : undefined,
+      prTitle: pr?.title,
+      prBaseHash: pr?.baseHash,
       buildHref: `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/actions/runs/${process.env.GITHUB_RUN_ID}`,
-      commitHash: process.env.GITHUB_SHA,
-      baseHash: pr ? pr.baseHash : process.env.GITHUB_BASE_REF,
-      branch: process.env.GITHUB_REF_NAME,
     };
   }
 
   if (process.env.GITLAB_CI) {
     return {
       commitHref: `${process.env.CI_PROJECT_URL}/-/commit/${process.env.CI_COMMIT_SHA}`,
-      prHref: process.env.CI_MERGE_REQUEST_IID ? `${process.env.CI_PROJECT_URL}/-/merge_requests/${process.env.CI_MERGE_REQUEST_IID}` : undefined,
-      buildHref: process.env.CI_JOB_URL,
       commitHash: process.env.CI_COMMIT_SHA,
-      baseHash: process.env.CI_COMMIT_BEFORE_SHA,
+      buildHref: process.env.CI_JOB_URL,
       branch: process.env.CI_COMMIT_REF_NAME,
     };
   }
@@ -95,7 +92,6 @@ async function ciInfo(): Promise<CIInfo | undefined> {
     return {
       commitHref: process.env.BUILD_URL,
       commitHash: process.env.GIT_COMMIT,
-      baseHash: process.env.GIT_PREVIOUS_COMMIT,
       branch: process.env.GIT_BRANCH,
     };
   }
@@ -144,13 +140,17 @@ async function gitCommitInfo(gitDir: string): Promise<GitCommitInfo | undefined>
 
 async function gitDiff(gitDir: string, ci?: CIInfo): Promise<string | undefined> {
   const diffLimit = 100_000;
-  if (ci?.baseHash) {
-    // First try the diff against the base branch.
-    const diff = await runGit(`git fetch origin ${ci.baseHash} && git diff ${ci.baseHash} HEAD`, gitDir);
+  if (ci?.prBaseHash) {
+    await runGit(`git fetch origin ${ci.prBaseHash}`, gitDir);
+    const diff = await runGit(`git diff ${ci.prBaseHash} HEAD`, gitDir);
     if (diff)
       return diff.substring(0, diffLimit);
   }
 
+  // Do not attempt to diff on CI commit.
+  if (ci)
+    return;
+
   // Check dirty state first.
   const uncommitted = await runGit('git diff', gitDir);
   if (uncommitted)

packages/playwright/src/prompt.ts

@@ -30,10 +30,24 @@ export async function attachErrorPrompts(testInfo: TestInfo, sourceCache: Map<st
   if (process.env.PLAYWRIGHT_NO_COPY_PROMPT)
     return;
 
+  const meaningfulSingleLineErrors = new Set(testInfo.errors.filter(e => e.message && !e.message.includes('\n')).map(e => e.message!));
+  for (const error of testInfo.errors) {
+    for (const singleLineError of meaningfulSingleLineErrors.keys()) {
+      if (error.message?.includes(singleLineError))
+        meaningfulSingleLineErrors.delete(singleLineError);
+    }
+  }
+
   for (const [index, error] of testInfo.errors.entries()) {
+    if (!error.message)
+      return;
     if (testInfo.attachments.find(a => a.name === `_prompt-${index}`))
       continue;
 
+    // Skip errors that are just a single line - they are likely to already be the error message.
+    if (!error.message.includes('\n') && !meaningfulSingleLineErrors.has(error.message))
+      continue;
+
     const metadata = testInfo.config.metadata as MetadataWithCommitInfo;
 
     const promptParts = [

packages/playwright/types/test.d.ts

@@ -960,11 +960,8 @@ interface TestConfig<TestArgs = {}, WorkerArgs = {}> {
   };
 
   /**
-   * - These settings control whether git information is captured and stored in the config
-   *   [testConfig.metadata](https://playwright.dev/docs/api/class-testconfig#test-config-metadata).
-   * - The structure of the git commit metadata is subject to change.
-   * - Default values for these settings depend on the environment. When tests run as a part of CI where it is safe to
-   *   obtain git information, the default value is true, false otherwise.
+   * These settings control whether git information is captured and stored in the config
+   * [testConfig.metadata](https://playwright.dev/docs/api/class-testconfig#test-config-metadata).
    *
    * **Usage**
    *
@@ -977,10 +974,20 @@ interface TestConfig<TestArgs = {}, WorkerArgs = {}> {
    * });
    * ```
    *
+   * **Details**
+   * - Capturing `commit` information is useful when you'd like to see it in your HTML (or a third party) report.
+   * - Capturing `diff` information is useful to enrich the report with the actual source diff. This information can
+   *   be used to provide intelligent advice on how to fix the test.
+   *
+   * **NOTE** Default values for these settings depend on the environment. When tests run as a part of CI where it is
+   * safe to obtain git information, the default value is `true`, `false` otherwise.
+   *
+   * **NOTE** The structure of the git commit metadata is subject to change.
+   *
    */
   captureGitInfo?: {
     /**
-     * Whether to capture commit information such as hash, author, timestamp.
+     * Whether to capture commit and pull request information such as hash, author, timestamp.
      */
     commit?: boolean;
 

tests/playwright-test/reporter-html.spec.ts

@@ -1232,10 +1232,7 @@ for (const useIntermediateMergeReport of [true, false] as const) {
 
       const result = await runInlineTest(files, { reporter: 'dot,html' }, {
         PLAYWRIGHT_HTML_OPEN: 'never',
-        GITHUB_ACTIONS: '1',
-        GITHUB_REPOSITORY: 'microsoft/playwright-example-for-test',
-        GITHUB_SERVER_URL: 'https://playwright.dev',
-        GITHUB_SHA: 'example-sha',
+        ...ghaCommitEnv(),
       });
 
       await showReport();
@@ -1262,23 +1259,9 @@ for (const useIntermediateMergeReport of [true, false] as const) {
       const baseDir = await writeFiles(files);
       await initGitRepo(baseDir);
 
-      const eventPath = path.join(baseDir, 'event.json');
-      await fs.promises.writeFile(eventPath, JSON.stringify({
-        pull_request: {
-          title: 'My PR',
-          number: 42,
-          base: { ref: 'main' },
-        },
-      }));
-
       const result = await runInlineTest(files, { reporter: 'dot,html' }, {
         PLAYWRIGHT_HTML_OPEN: 'never',
-        GITHUB_ACTIONS: '1',
-        GITHUB_REPOSITORY: 'microsoft/playwright-example-for-test',
-        GITHUB_RUN_ID: 'example-run-id',
-        GITHUB_SERVER_URL: 'https://playwright.dev',
-        GITHUB_SHA: 'example-sha',
-        GITHUB_EVENT_PATH: eventPath,
+        ...(await ghaPullRequestEnv(baseDir))
       });
 
       await showReport();
@@ -2746,31 +2729,34 @@ for (const useIntermediateMergeReport of [true, false] as const) {
       await expect(page.locator('.tree-item', { hasText: 'stdout' })).toHaveCount(1);
     });
 
-    test('should show AI prompt', async ({ runInlineTest, writeFiles, showReport, page }) => {
+    test('should include diff in AI prompt', async ({ runInlineTest, writeFiles, showReport, page }) => {
       const files = {
         'uncommitted.txt': `uncommitted file`,
         'playwright.config.ts': `export default {}`,
         'example.spec.ts': `
           import { test, expect } from '@playwright/test';
           test('sample', async ({ page }) => {
             await page.setContent('<button>Click me</button>');
-            expect(2).toBe(3);
+            expect(2).toBe(2);
           });
         `,
       };
       const baseDir = await writeFiles(files);
       await initGitRepo(baseDir);
+      await writeFiles({
+        'example.spec.ts': `
+          import { test, expect } from '@playwright/test';
+          test('sample', async ({ page }) => {
+            await page.setContent('<button>Click me</button>');
+            expect(2).toBe(3);
+          });`
+      });
+      await execGit(baseDir, ['checkout', '-b', 'pr_branch']);
+      await execGit(baseDir, ['commit', '-am', 'changes']);
 
-      const result = await runInlineTest(files, { reporter: 'dot,html' }, {
+      const result = await runInlineTest({}, { reporter: 'dot,html' }, {
         PLAYWRIGHT_HTML_OPEN: 'never',
-        GITHUB_ACTIONS: '1',
-        GITHUB_REPOSITORY: 'microsoft/playwright-example-for-test',
-        GITHUB_RUN_ID: 'example-run-id',
-        GITHUB_SERVER_URL: 'https://playwright.dev',
-        GITHUB_SHA: 'example-sha',
-        GITHUB_REF_NAME: '42/merge',
-        GITHUB_BASE_REF: 'HEAD~1',
-        PLAYWRIGHT_COPY_PROMPT: '1',
+        ...(await ghaPullRequestEnv(baseDir)),
       });
 
       expect(result.exitCode).toBe(1);
@@ -2786,6 +2772,24 @@ for (const useIntermediateMergeReport of [true, false] as const) {
       expect(prompt, 'contains snapshot').toContain('- button "Click me"');
       expect(prompt, 'contains diff').toContain(`+            expect(2).toBe(3);`);
     });
+
+    test('should not show prompt for empty timeout error', async ({ runInlineTest, showReport, page }) => {
+      const result = await runInlineTest({
+        'uncommitted.txt': `uncommitted file`,
+        'playwright.config.ts': `export default {}`,
+        'example.spec.ts': `
+          import { test, expect } from '@playwright/test';
+          test('sample', async ({ page }) => {
+            test.setTimeout(2000);
+            await page.setChecked('input', true);
+          });
+        `,
+      }, { reporter: 'dot,html' }, { PLAYWRIGHT_HTML_OPEN: 'never' });
+      expect(result.exitCode).toBe(1);
+      await showReport();
+      await page.getByRole('link', { name: 'sample' }).click();
+      await expect(page.getByRole('button', { name: 'Copy prompt' })).toHaveCount(1);
+    });
   });
 }
 
@@ -2797,19 +2801,45 @@ function readAllFromStream(stream: NodeJS.ReadableStream): Promise<Buffer> {
   });
 }
 
-async function initGitRepo(baseDir) {
-  const execGit = async (args: string[]) => {
-    const { code, stdout, stderr } = await spawnAsync('git', args, { stdio: 'pipe', cwd: baseDir });
-    if (!!code)
-      throw new Error(`Non-zero exit of:\n$ git ${args.join(' ')}\nConsole:\nstdout:\n${stdout}\n\nstderr:\n${stderr}\n\n`);
-    return;
+async function execGit(baseDir: string, args: string[]) {
+  const { code, stdout, stderr } = await spawnAsync('git', args, { stdio: 'pipe', cwd: baseDir });
+  if (!!code)
+    throw new Error(`Non-zero exit of:\n$ git ${args.join(' ')}\nConsole:\nstdout:\n${stdout}\n\nstderr:\n${stderr}\n\n`);
+  return;
+}
+
+async function initGitRepo(baseDir: string) {
+  await execGit(baseDir, ['init']);
+  await execGit(baseDir, ['config', '--local', 'user.email', '[email protected]']);
+  await execGit(baseDir, ['config', '--local', 'user.name', 'William']);
+  await execGit(baseDir, ['checkout', '-b', 'main']);
+  await execGit(baseDir, ['add', 'playwright.config.ts']);
+  await execGit(baseDir, ['commit', '-m', 'init']);
+  await execGit(baseDir, ['add', '*.ts']);
+  await execGit(baseDir, ['commit', '-m', 'chore(html): make this test look nice']);
+}
+
+function ghaCommitEnv() {
+  return {
+    GITHUB_ACTIONS: '1',
+    GITHUB_REPOSITORY: 'microsoft/playwright-example-for-test',
+    GITHUB_SERVER_URL: 'https://playwright.dev',
+    GITHUB_SHA: 'example-sha',
   };
+}
 
-  await execGit(['init']);
-  await execGit(['config', '--local', 'user.email', '[email protected]']);
-  await execGit(['config', '--local', 'user.name', 'William']);
-  await execGit(['add', 'playwright.config.ts']);
-  await execGit(['commit', '-m', 'init']);
-  await execGit(['add', '*.ts']);
-  await execGit(['commit', '-m', 'chore(html): make this test look nice']);
+async function ghaPullRequestEnv(baseDir: string) {
+  const eventPath = path.join(baseDir, 'event.json');
+  await fs.promises.writeFile(eventPath, JSON.stringify({
+    pull_request: {
+      title: 'My PR',
+      number: 42,
+      base: { sha: 'main' },
+    },
+  }));
+  return {
+    ...ghaCommitEnv(),
+    GITHUB_RUN_ID: 'example-run-id',
+    GITHUB_EVENT_PATH: eventPath,
+  };
 }

tests/playwright-test/ui-mode-llm.spec.ts

@@ -46,7 +46,6 @@ test.fixme('openai', async ({ runUITest, server }) => {
   }, {
     EXPERIMENTAL_OPENAI_API_KEY: 'fake-key',
     OPENAI_BASE_URL: server.PREFIX,
-    PLAYWRIGHT_COPY_PROMPT: '1',
   });
 
   await page.getByTitle('Run all').click();
@@ -87,7 +86,6 @@ test.fixme('anthropic', async ({ runUITest, server }) => {
   }, {
     EXPERIMENTAL_ANTHROPIC_API_KEY: 'fake-key',
     ANTHROPIC_BASE_URL: server.PREFIX,
-    PLAYWRIGHT_COPY_PROMPT: '1',
   });
 
   await page.getByTitle('Run all').click();

tests/playwright-test/ui-mode-trace.spec.ts

@@ -508,7 +508,7 @@ test('fails', async ({ page }) => {
   expect(1).toBe(2);
 });
     `.trim(),
-  }, { PLAYWRIGHT_COPY_PROMPT: '1' });
+  });
 
   await page.getByText('fails').dblclick();