Skip to content
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

Fix flaky completion test and clean up console #1864

Merged
merged 4 commits into from
Nov 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions vscode/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Starting from `0.2.0`, Cody is using `major.EVEN_NUMBER.patch` for release versi
- Chat: Input history is now preserved between chat sessions. [pull/1826](https://github.com/sourcegraph/cody/pull/1826)
- Chat: Fixed chat command selection behavior in chat input box. [pull/1828](https://github.com/sourcegraph/cody/pull/1828)
- Chat: Add delays before sending webview ready events to prevent premature sending. This fixes issue where chat panel fails to load when multiple chat panels are opened simultaneously. [pull/1836](https://github.com/sourcegraph/cody/pull/1836)
- Autocomplete: Fixes a bug that caused autocomplete to be triggered at the end of a block or function invocation. [pull/1864](https://github.com/sourcegraph/cody/pull/1864)

### Changed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,10 @@ describe('[getInlineCompletions] triggers', () => {
})

describe('closing symbols', () => {
it.each(['{}█', '[]█', '()█'])('does not trigger for %s', async prompt =>
it.each(['{}█', '[]█', '()█', ';█'])('does not trigger for %s', async prompt =>
expect(await getInlineCompletions(params(prompt, [completion`bar`]))).toEqual<V>(null)
)
it.each(['{}\n█', '[]\n█', '()\n█'])('does trigger for %s', async prompt =>
it.each(['{}\n█', '[]\n█', '()\n█', ';\n█'])('does trigger for %s', async prompt =>
expect(await getInlineCompletions(params(prompt, [completion`bar`]))).toEqual<V>({
items: [{ insertText: 'bar' }],
source: InlineCompletionsResultSource.Network,
Expand Down
2 changes: 1 addition & 1 deletion vscode/src/completions/get-inline-completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ async function doGetInlineCompletions(params: InlineCompletionsParams): Promise<
}

// Do not trigger when the last character is a closing symbol
if (triggerKind !== TriggerKind.Manual && /[)\]}]$/.test(currentLinePrefix.trim())) {
if (triggerKind !== TriggerKind.Manual && /[);\]}]$/.test(currentLinePrefix.trim())) {
Copy link
Contributor Author

@philipp-spiess philipp-spiess Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to sneak this in anyways; If we have a line like this:

foo.bar();

we don't want to trigger a completion regardless of wether we print a semi or not.

return null
}

Expand Down
7 changes: 6 additions & 1 deletion vscode/test/e2e/inline-completion.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ test('inline completion onboarding notice on first completion accept', async ({
'CodyVSCodeExtension:completion:suggested', // Suggestion that appears immediately after accepting
'CodyVSCodeExtension:completion:suggested', // Second suggestion after typing "a" to test hiding
'CodyVSCodeExtension:completion:accepted', // Second accept
'CodyVSCodeExtension:completion:suggested', // Suggestion that appears immediately after accepting
]

const indexFile = page.getByRole('treeitem', { name: 'index.html' }).locator('a')
Expand Down Expand Up @@ -103,8 +102,14 @@ test('inline completion onboarding notice on first completion accept', async ({
// Trigger/accept another completion, but don't expect the notification.
await triggerInlineCompletionAfter(page, firstAcceptedCompletion)
await acceptInlineCompletion(page)
// After accepting a completion, a new completion request will be made. Since this can interfere
// with the expected event order (especially since suggestion events are logged after the
// completion is hidden), we type a semicolon which will prevent an automatic completion from
// showing up
await page.keyboard.press(';')
await expect(otherAcceptedCompletion).toBeVisible()
await expect(decoration).not.toBeVisible()

await assertEvents(loggedEvents, expectedEvents)
})

Expand Down
14 changes: 12 additions & 2 deletions vscode/test/e2e/install-deps.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,21 @@
import { spawn } from 'child_process'

import { downloadAndUnzipVSCode } from '@vscode/test-electron'
import { ConsoleReporter, downloadAndUnzipVSCode, ProgressReport, ProgressReportStage } from '@vscode/test-electron'

export const vscodeVersion = '1.81.1'

// A custom version of the VS Code download reporter that silences matching installation
// notifications as these otherwise are emitted on every test run
class CustomConsoleReporter extends ConsoleReporter {
public report(report: ProgressReport): void {
if (report.stage !== ProgressReportStage.FoundMatchingInstall) {
return super.report(report)
}
}
}

export function installVsCode(): Promise<string> {
return downloadAndUnzipVSCode(vscodeVersion)
return downloadAndUnzipVSCode(vscodeVersion, undefined, new CustomConsoleReporter(process.stdout.isTTY))
}

export function installChromium(): Promise<void> {
Expand Down
8 changes: 5 additions & 3 deletions vscode/test/fixtures/mock-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,7 @@ export async function run<T>(around: () => Promise<T>): Promise<T> {
}
})

const server = app.listen(SERVER_PORT, () => {
console.log(`Mock server listening on port ${SERVER_PORT}`)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not helpful. Tests should only log something if something fails IMO

})
const server = app.listen(SERVER_PORT)

const result = await around()

Expand All @@ -135,6 +133,10 @@ export async function run<T>(around: () => Promise<T>): Promise<T> {
}

export async function logTestingData(type: 'legacy' | 'new', data: string): Promise<void> {
if (process.env.CI === undefined) {
return
}
Comment on lines +136 to +138
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akalia25 is it okay if this runs only on our CI servers? Otherwise this makes local development a PITA

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course! I appreciate you checking 🙇


const message = {
type,
event: data,
Expand Down