-
Notifications
You must be signed in to change notification settings - Fork 37.6k
Add WSL sanity tests #290682
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
Add WSL sanity tests #290682
Conversation
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
This PR adds comprehensive WSL (Windows Subsystem for Linux) sanity tests to the VS Code test suite.
Changes:
- Adds new WSL test file with server, server-web, and desktop tests for both ARM64 and x64 architectures
- Extends TestContext with WSL-specific helper methods (path conversion, temp directory management, distro detection)
- Adds WSL capability detection to the detectors system
- Refactors existing tests to use centralized
getPage()andfetchNoErrors()helper methods for consistency - Increases default page timeout from 2 to 3 minutes to accommodate potentially slower WSL operations
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/sanity/src/wsl.test.ts | New file containing WSL-specific tests for server, server-web, and desktop scenarios with a custom WslUITest class |
| test/sanity/src/context.ts | Adds WSL helper methods (createWslTempDir, deleteWslDir, toWslPath, getDefaultWslDistro, getWslServerExtensionsDir, getPage), updates getServerEntryPoint to support WSL |
| test/sanity/src/uiTest.ts | Changes context and verification methods from private to protected to allow WslUITest subclass to override them |
| test/sanity/src/detectors.ts | Adds 'wsl' capability type and detectWSL() function to check for WSL availability on Windows |
| test/sanity/src/main.ts | Imports and sets up WSL tests |
| test/sanity/src/serverWeb.test.ts | Refactors to use context.getPage() helper |
| test/sanity/src/server.test.ts | Refactors to use context.fetchNoErrors() helper |
| test/sanity/src/desktop.test.ts | Refactors to use context.getPage() helper |
Comments suppressed due to low confidence (2)
test/sanity/src/wsl.test.ts:135
- The spawn call is missing the
detachedoption that's used in similar patterns throughout the codebase. In server.test.ts line 84, the pattern usesconst detached = !context.capabilities.has('windows')and passes it to spawn options. While WSL tests always run on Windows (where detached should be false), it would be more consistent with the codebase pattern to explicitly set this option for clarity:{ shell: true, detached: false }
const server = spawn('wsl', [wslPath, ...args], { shell: true });
test/sanity/src/wsl.test.ts:238
- Using
context.runwithout checking if the command succeeded. If the WSL command fails, the test will continue with potentially undefined behavior. Consider checkingresult.statusor usingcontext.runNoErrorsinstead to ensure the command succeeds. Looking at the codebase pattern in cli.test.ts line 74, similar commands userunNoErrorsfor guaranteed success.
const result = this.context.run('wsl', 'cat', `${this.wslWorkspaceDir}/helloWorld.txt`);
assert.strictEqual(result.stdout.trim(), 'Hello, World!', 'File contents in WSL do not match expected value');
| const result = this.context.run('wsl', 'cat', `${this.wslWorkspaceDir}/helloWorld.txt`); | ||
| assert.strictEqual(result.stdout.trim(), 'Hello, World!', 'File contents in WSL do not match expected value'); | ||
| } | ||
|
|
||
| protected override verifyExtensionInstalled() { | ||
| this.context.log(`Verifying extension is installed in WSL at ${this.wslExtensionsDir}`); | ||
| const result = this.context.run('wsl', 'ls', this.wslExtensionsDir); |
Copilot
AI
Jan 27, 2026
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.
Using context.run without checking if the command succeeded. If the WSL command fails, the test will continue with potentially undefined behavior. Consider checking result.status or using context.runNoErrors instead to ensure the command succeeds. Looking at the codebase pattern in cli.test.ts line 74, similar commands use runNoErrors for guaranteed success.
| const result = this.context.run('wsl', 'cat', `${this.wslWorkspaceDir}/helloWorld.txt`); | |
| assert.strictEqual(result.stdout.trim(), 'Hello, World!', 'File contents in WSL do not match expected value'); | |
| } | |
| protected override verifyExtensionInstalled() { | |
| this.context.log(`Verifying extension is installed in WSL at ${this.wslExtensionsDir}`); | |
| const result = this.context.run('wsl', 'ls', this.wslExtensionsDir); | |
| const result = this.context.runNoErrors('wsl', 'cat', `${this.wslWorkspaceDir}/helloWorld.txt`); | |
| assert.strictEqual(result.stdout.trim(), 'Hello, World!', 'File contents in WSL do not match expected value'); | |
| } | |
| protected override verifyExtensionInstalled() { | |
| this.context.log(`Verifying extension is installed in WSL at ${this.wslExtensionsDir}`); | |
| const result = this.context.runNoErrors('wsl', 'ls', this.wslExtensionsDir); |
| */ | ||
| public deleteWslDir(dir: string): void { | ||
| this.log(`Deleting WSL directory: ${dir}`); | ||
| this.run('wsl', 'rm', '-rf', dir); |
Copilot
AI
Jan 27, 2026
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.
Using context.run without checking if the command succeeded. If the WSL rm command fails during cleanup, the error will be silently ignored. While this might be acceptable for cleanup operations in a try-catch block (as seen at line 227), it would be better to at least log the stderr output if the command fails. Consider adding error checking: if (result.status !== 0) { this.log(\Failed to delete WSL dir: ${result.stderr}`); }`
| this.run('wsl', 'rm', '-rf', dir); | |
| const result = this.run('wsl', 'rm', '-rf', dir); | |
| if (result.status !== 0) { | |
| this.log(`Failed to delete WSL directory ${dir}: ${result.stderr}`); | |
| } |
| ]; | ||
|
|
||
| context.log(`Starting server in WSL: ${wslPath} with args ${args.join(' ')}`); | ||
| const server = spawn('wsl', [wslPath, ...args], { shell: true }); |
Copilot
AI
Jan 27, 2026
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.
The spawn call is missing the detached option that's used in similar patterns throughout the codebase. In server.test.ts line 84, the pattern uses const detached = !context.capabilities.has('windows') and passes it to spawn options. While WSL tests always run on Windows (where detached should be false), it would be more consistent with the codebase pattern to explicitly set this option for clarity: { shell: true, detached: false }
| const server = spawn('wsl', [wslPath, ...args], { shell: true }); | |
| const server = spawn('wsl', [wslPath, ...args], { shell: true, detached: false }); |
| const app = await _electron.launch({ executablePath: entryPoint, args }); | ||
| const window = await context.getPage(app.firstWindow()); | ||
|
|
||
| context.log('Installing WPS extension'); |
Copilot
AI
Jan 27, 2026
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.
The comment says "Installing WPS extension" but based on the context of the codebase (UITest class installs GitHub Pull Requests extension), this should say "Installing WSL extension" or more specifically the extension that enables WSL connectivity.
| context.log('Installing WPS extension'); | |
| context.log('Installing WSL extension'); |
Covers these scenarios: