From 9e9f0d17e1051cc6cc6d0db1b4608445e1d13377 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 5 Mar 2025 20:43:27 -0800 Subject: [PATCH] cherry-pick(#35062): do not compute git diff on non! PRs --- docs/src/test-api/class-testconfig.md | 20 ++- packages/playwright/src/isomorphic/types.d.ts | 2 +- .../src/plugins/gitCommitInfoPlugin.ts | 22 ++-- packages/playwright/src/prompt.ts | 14 +++ packages/playwright/types/test.d.ts | 19 ++- tests/playwright-test/reporter-html.spec.ts | 116 +++++++++++------- tests/playwright-test/ui-mode-llm.spec.ts | 2 - tests/playwright-test/ui-mode-trace.spec.ts | 2 +- 8 files changed, 129 insertions(+), 68 deletions(-) diff --git a/docs/src/test-api/class-testconfig.md b/docs/src/test-api/class-testconfig.md index d6ec36edd7349..c2d29d0eb40f2 100644 --- a/docs/src/test-api/class-testconfig.md +++ b/docs/src/test-api/class-testconfig.md @@ -39,12 +39,10 @@ export default defineConfig({ ## property: TestConfig.captureGitInfo * since: v1.51 - type: ?<[Object]> - - `commit` ? Whether to capture commit information such as hash, author, timestamp. + - `commit` ? Whether to capture commit and pull request information such as hash, author, timestamp. - `diff` ? 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]> diff --git a/packages/playwright/src/isomorphic/types.d.ts b/packages/playwright/src/isomorphic/types.d.ts index 6c211f3594f43..5f31b925a2e3f 100644 --- a/packages/playwright/src/isomorphic/types.d.ts +++ b/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; }; diff --git a/packages/playwright/src/plugins/gitCommitInfoPlugin.ts b/packages/playwright/src/plugins/gitCommitInfoPlugin.ts index dd1f2695a9f69..463b6d7aacbea 100644 --- a/packages/playwright/src/plugins/gitCommitInfoPlugin.ts +++ b/packages/playwright/src/plugins/gitCommitInfoPlugin.ts @@ -71,22 +71,19 @@ async function ciInfo(): Promise { 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 { 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 async function gitDiff(gitDir: string, ci?: CIInfo): Promise { 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) diff --git a/packages/playwright/src/prompt.ts b/packages/playwright/src/prompt.ts index 840cbe73dabd7..141e4a4d5bd3c 100644 --- a/packages/playwright/src/prompt.ts +++ b/packages/playwright/src/prompt.ts @@ -30,10 +30,24 @@ export async function attachErrorPrompts(testInfo: TestInfo, sourceCache: Map 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 = [ diff --git a/packages/playwright/types/test.d.ts b/packages/playwright/types/test.d.ts index 19ab77c303fa7..1d95350d0d9d6 100644 --- a/packages/playwright/types/test.d.ts +++ b/packages/playwright/types/test.d.ts @@ -960,11 +960,8 @@ interface TestConfig { }; /** - * - 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 { * }); * ``` * + * **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; diff --git a/tests/playwright-test/reporter-html.spec.ts b/tests/playwright-test/reporter-html.spec.ts index cf84a65cc50e9..727cbe4077547 100644 --- a/tests/playwright-test/reporter-html.spec.ts +++ b/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,7 +2729,7 @@ 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 {}`, @@ -2754,23 +2737,26 @@ for (const useIntermediateMergeReport of [true, false] as const) { import { test, expect } from '@playwright/test'; test('sample', async ({ page }) => { await page.setContent(''); - 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(''); + 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 { }); } -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', 'shakespeare@example.local']); + 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', 'shakespeare@example.local']); - 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, + }; } diff --git a/tests/playwright-test/ui-mode-llm.spec.ts b/tests/playwright-test/ui-mode-llm.spec.ts index 27f5c91aebecb..bc2cafe40edb8 100644 --- a/tests/playwright-test/ui-mode-llm.spec.ts +++ b/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(); diff --git a/tests/playwright-test/ui-mode-trace.spec.ts b/tests/playwright-test/ui-mode-trace.spec.ts index 71b63da16ed34..274dbd644b5ef 100644 --- a/tests/playwright-test/ui-mode-trace.spec.ts +++ b/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();