-
Notifications
You must be signed in to change notification settings - Fork 238
chore(vscode): add test for quickfix #4958
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
base: main
Are you sure you want to change the base?
Conversation
benfdking
commented
Jul 11, 2025
- also refactored the openProblemsView
- also refactored the openProblemsView
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.
Pull Request Overview
Adds a new end-to-end test for the “noselectstar” quickfix and centralizes opening the Problems panel via a shared helper.
- Introduce
openProblemsView
utility inutils.ts
- Replace inline
runCommand(page, 'View: Focus Problems')
calls withopenProblemsView
across tests - Add
quickfix.spec.ts
to validate the quickfix replacesSELECT *
with explicit columns
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
vscode/extension/tests/utils.ts | Added openProblemsView helper with JSDoc |
vscode/extension/tests/quickfix.spec.ts | New test for the noselectstar quickfix |
vscode/extension/tests/diagnostics.spec.ts | Swapped runCommand for openProblemsView |
vscode/extension/tests/broken_project.spec.ts | Replaced a single runCommand call with openProblemsView |
Comments suppressed due to low confidence (1)
vscode/extension/tests/utils.ts:89
- [nitpick] The JSDoc for
openProblemsView
is missing a@param
annotation forpage
; adding parameter documentation improves clarity for users of this helper.
/**
.first() | ||
.click() | ||
|
||
await page.waitForTimeout(1_000) |
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.
Avoid using a fixed timeout for waiting; prefer waiting on a specific condition (e.g., expect.poll
or waitForFunction
) to detect when the file has been updated.
await page.waitForTimeout(1_000) | |
await page.waitForFunction(async () => { | |
const content = await fs.readFile(modelPath, 'utf8'); | |
return content.includes('SELECT id, customer_id, waiter_id, start_ts, end_ts, event_date') && !content.includes('SELECT *'); | |
}); |
Copilot uses AI. Check for mistakes.
const readUpdatedFile = await fs.readFile(modelPath) | ||
expect(readUpdatedFile.toString()).not.toContain('SELECT *') | ||
expect(readUpdatedFile.toString()).toContain( |
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.
[nitpick] The file is read and converted to string multiple times for assertions; consider reading once and storing the string in a variable to simplify and clarify the checks.
const readUpdatedFile = await fs.readFile(modelPath) | |
expect(readUpdatedFile.toString()).not.toContain('SELECT *') | |
expect(readUpdatedFile.toString()).toContain( | |
const readUpdatedFile = (await fs.readFile(modelPath)).toString() | |
expect(readUpdatedFile).not.toContain('SELECT *') | |
expect(readUpdatedFile).toContain( |
Copilot uses AI. Check for mistakes.