-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
autoedits e2e tests #6425
autoedits e2e tests #6425
Conversation
b4a4af8
to
d1763d3
Compare
ae45dbd
to
b3583e4
Compare
@@ -120,6 +124,14 @@ export class AutoeditsProvider implements vscode.InlineCompletionItemProvider, v | |||
vscode.window.onDidChangeTextEditorSelection(this.onSelectionChangeDebounced), | |||
vscode.workspace.onDidChangeTextDocument(event => { | |||
this.onDidChangeTextDocument(event) | |||
}), | |||
// Command used to trigger autoedits manually via command palette and is used by e2e test | |||
vscode.commands.registerCommand('cody.command.autoedits-manual-trigger', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's register this command in the registerAutoEdits
function body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
}), | ||
// Command used to trigger autoedits manually via command palette and is used by e2e test | ||
vscode.commands.registerCommand('cody.command.autoedits-manual-trigger', async () => { | ||
this.showAutoEdit( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using an approach similar to autocomplete? This would invoke the provideInlineCompletionItems
method with appropriate parameters.
public async manuallyTriggerCompletion(): Promise<void> {
await vscode.commands.executeCommand('editor.action.inlineSuggest.hide')
this.lastManualCompletionTimestamp = Date.now()
await vscode.commands.executeCommand('editor.action.inlineSuggest.trigger')
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice !!
This is much better, changed to the proposed implementation
* | ||
* Currently these tests run only on macOS due to cross-platform rendering inconsistencies. | ||
* | ||
* Previous attempts and challenges encountered: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 🤩
""" | ||
<<<< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep E2E tests closer to the real-world setup, we usually mock server responses in vscode/test/fixtures/mock-server.ts.
Do you think we can do that for autoedits? If we rely on the /.api/completions/code
endpoint for suggestions generation, it's already mocked in the mock server implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first approach I tried when implementing e2e tests. There are the painpoints in using the mock-server
:
- The current
mock-server
has a hardcoded list of responses, which we check on case to case basis ior the mock list in case of./api/completions/code
endpoint, based on the content on the prompt. This setup is not very extensible if we want to add say 10-12 different image types from cody-chat-eval.
I think the current setup has a downside that I doesn't do thought the adapter layer in e2e setup, but this gives us much more flexibility in adding new test cases and simply port over the examples from cody-chat-eval. - Another pain-point would be that, the response returned from the server is a rewrite of
<code_to_rewrite>
, so in the hardcoded list above, we need to extract the logic either manually or we need to use the same setupi.e. mock-renderer
, so in any case we would use the existingmock-renderer
setup to get the response and extend the mock response list.
I think the current adapters are covered with extensive unit tests and should be fine to skip it in the e2e tests and follow the mock responses. Do you think we should continue with the setup we have in the PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing the context! I agree; we can change it in a follow-up if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you leave this context as a comment in the test file for posterity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, Added detailed comment in the test file
// Wait for the diff view to stabilize - required to reduce flakiness | ||
await page.waitForTimeout(500) | ||
|
||
await expect(page).toHaveScreenshot([snapshotPlatform, snapshotName], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥✨
vscode/test/e2e/auto-edits.test.ts
Outdated
await expect(page).toHaveScreenshot([snapshotPlatform, snapshotName], { | ||
maxDiffPixelRatio: 0.02, | ||
maxDiffPixels: 1000, | ||
clip: clip, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clip: clip, | |
clip, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Thank you for implementing these tests.
## Context 1. Adding unit test case for the renderer manager 2. Build on top of #6425 ## Test plan Added unit test
1. Adding unit test case for the renderer manager 2. Build on top of #6425 Added unit test
477bf1a
to
30ce79c
Compare
Context
auto-edits
Test plan
Added unit test and e2e tests