Add integration test for extensions#9177
Conversation
Summary of ChangesHello @chrstnb, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes foundational integration testing for the extension management features. It adds a new test file that simulates the lifecycle of a local extension, from installation and verification to updating and uninstallation. To facilitate this, the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Size Change: -2 B (0%) Total Size: 17.4 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Code Review
This pull request introduces the first integration test for extension management, which is a valuable addition for ensuring the stability of these features. The changes also include a new helper method in TestRig for executing commands. My review focuses on improving the test's robustness and the maintainability of the test helper code. I've identified some duplicated logic that should be refactored and opportunities to make the tests more isolated and the test rig API safer to use.
| runCommand( | ||
| args: string[], | ||
| options: { stdin?: string } = {}, | ||
| ): Promise<string> { | ||
| const commandArgs = [this.bundlePath, ...args]; | ||
|
|
||
| const child = spawn('node', commandArgs, { | ||
| cwd: this.testDir!, | ||
| stdio: 'pipe', | ||
| }); | ||
|
|
||
| let stdout = ''; | ||
| let stderr = ''; | ||
|
|
||
| if (options.stdin) { | ||
| child.stdin!.write(options.stdin); | ||
| child.stdin!.end(); | ||
| } | ||
|
|
||
| child.stdout!.on('data', (data: Buffer) => { | ||
| stdout += data; | ||
| if (env.KEEP_OUTPUT === 'true' || env.VERBOSE === 'true') { | ||
| process.stdout.write(data); | ||
| } | ||
| }); | ||
|
|
||
| child.stderr!.on('data', (data: Buffer) => { | ||
| stderr += data; | ||
| if (env.KEEP_OUTPUT === 'true' || env.VERBOSE === 'true') { | ||
| process.stderr.write(data); | ||
| } | ||
| }); | ||
|
|
||
| const promise = new Promise<string>((resolve, reject) => { | ||
| child.on('close', (code: number) => { | ||
| if (code === 0) { | ||
| this._lastRunStdout = stdout; | ||
| let result = stdout; | ||
| if (stderr) { | ||
| result += `\n\nStdErr:\n${stderr}`; | ||
| } | ||
| resolve(result); | ||
| } else { | ||
| reject(new Error(`Process exited with code ${code}:\n${stderr}`)); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| return promise; | ||
| } |
There was a problem hiding this comment.
The new runCommand method duplicates a significant amount of logic from the existing run method (lines 178-307). This includes process spawning, handling stdout and stderr streams, and managing the child process lifecycle with a Promise. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, this common logic should be extracted into a private helper method. Both run and runCommand can then call this helper and apply their specific argument processing and output handling.
TLDR
Adding the first (simple) integration test for installing, updating, and listing a local extension
Dive Deeper
Reviewer Test Plan
Testing Matrix
Linked issues / bugs