feat: provide page snapshot as context to copilot #610
Conversation
| @@ -551,6 +551,7 @@ export class TestModel extends DisposableBase { | |||
| connectWsEndpoint: showBrowser ? externalOptions.connectWsEndpoint : undefined, | |||
| updateSnapshots: noOverrideToUndefined(this._embedder.settingsModel.updateSnapshots.get()), | |||
| updateSourceMethod: noOverrideToUndefined(this._embedder.settingsModel.updateSourceMethod.get()), | |||
| pageSnapshot: 'only-on-failure', | |||
There was a problem hiding this comment.
if we release with this as the default in @playwright/test, we can remove this override again.
src/extension.ts
Outdated
| @@ -520,7 +520,7 @@ export class Extension implements RunHooks { | |||
| } | |||
| }, | |||
|
|
|||
| onTestEnd: (test: reporterTypes.TestCase, result: reporterTypes.TestResult) => { | |||
| onTestEnd: async (test: reporterTypes.TestCase, result: reporterTypes.TestResult) => { | |||
There was a problem hiding this comment.
I don't think we are comfortable making onTestEnd async.
There was a problem hiding this comment.
Hmm, that's unfortunate. If we read the attachment from disk, that needs to be asynchronous. Do you think it'd be OK if we report only the test failures asynchronously?
There was a problem hiding this comment.
how do you like 2dba9b5? Or should we make it an asynchronous inline function so the code stays colocated?
src/extension.ts
Outdated
| const snapshot = result.attachments.find(a => a.name === 'pageSnapshot'); | ||
| if (snapshot && snapshot.path) { | ||
| const contents = await this._vscode.workspace.fs.readFile(this._vscode.Uri.file(snapshot.path)); | ||
| aiContext = `### Page Snapshot at Failure\n${contents.toString()}`; // cannot use ``` codeblocks, vscode markdown does not support it |
There was a problem hiding this comment.
Don't you need two \n to make a paragraph?
There was a problem hiding this comment.
some renderers do, you're right. vscode didn't require it for some reason, but it still makes sense to make it two newlines in case vscode markdown rendering ever changes.
| let aiContext: string | undefined; | ||
| const snapshot = result.attachments.find(a => a.name === 'pageSnapshot'); | ||
| if (snapshot && snapshot.path) { | ||
| const contents = await this._vscode.workspace.fs.readFile(this._vscode.Uri.file(snapshot.path)); |
There was a problem hiding this comment.
I wonder whether we should either limit the number of files we read here, or make pageSnapshot attachment content-based, not file-based.
There was a problem hiding this comment.
It only ever reads one file per test, which should be fine.
If we make this content-based, there's a couple downsides:
- it'll auto-print to terminal on some reporters, which is very noisy
- it'll take up more memory for any consumer that holds on to old snapshots
Also, AI tools like Copilot won't be able to discover it from the file system. not yet sure if this a good or bad thing.
Enables
pageSnapshot: 'only-on-failure'for all test runs through the extension, and injects the page snapshot into the error message that Copilot uses to fix test failures.related to microsoft/playwright#34476