Skip to content

Commit

Permalink
cherry-pick(#35062): do not compute git diff on non! PRs
Browse files Browse the repository at this point in the history
  • Loading branch information
pavelfeldman committed Mar 6, 2025
1 parent 12835d6 commit 9e9f0d1
Show file tree
Hide file tree
Showing 8 changed files with 129 additions and 68 deletions.
20 changes: 16 additions & 4 deletions docs/src/test-api/class-testconfig.md
Original file line number Diff line number Diff line change
Expand Up @@ -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**

Expand All @@ -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]>
Expand Down
2 changes: 1 addition & 1 deletion packages/playwright/src/isomorphic/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ export type CIInfo = {
commitHref: string;
prHref?: string;
prTitle?: string;
prBaseHash?: string;
buildHref?: string;
commitHash?: string;
baseHash?: string;
branch?: string;
};

Expand Down
22 changes: 11 additions & 11 deletions packages/playwright/src/plugins/gitCommitInfoPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}
Expand All @@ -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,
};
}
Expand Down Expand Up @@ -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)
Expand Down
14 changes: 14 additions & 0 deletions packages/playwright/src/prompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down
19 changes: 13 additions & 6 deletions packages/playwright/types/test.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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**
*
Expand All @@ -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;

Expand Down
116 changes: 73 additions & 43 deletions tests/playwright-test/reporter-html.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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);
Expand All @@ -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);
});
});
}

Expand All @@ -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,
};
}
2 changes: 0 additions & 2 deletions tests/playwright-test/ui-mode-llm.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion tests/playwright-test/ui-mode-trace.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ test('fails', async ({ page }) => {
expect(1).toBe(2);
});
`.trim(),
}, { PLAYWRIGHT_COPY_PROMPT: '1' });
});

await page.getByText('fails').dblclick();

Expand Down

0 comments on commit 9e9f0d1

Please sign in to comment.