Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability of VS Code sanity tests by implementing better UI waiting mechanisms, enhanced error handling, and fixes for x64 macOS test execution. The changes refactor test code to be more maintainable and add retry logic for download operations.
Changes:
- Replaced keyboard shortcuts with command palette commands for better cross-platform reliability
- Added retry logic with exponential backoff for network fetch operations
- Refactored server test code to extract reusable test functions
- Added support for universal binary testing on x64 macOS with proper architecture preference
- Changed macOS app entry point from CLI executable to Electron executable
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/sanity/src/uiTest.ts | Refactored UI interactions to use command palette instead of keyboard shortcuts; improved element selectors for better reliability |
| test/sanity/src/serverWeb.test.ts | Extracted runUITest helper function; replaced hardcoded token with random token generation |
| test/sanity/src/server.test.ts | Extracted runWebTest helper function; added random port and token generation |
| test/sanity/src/main.ts | Added beforeEach hook to change working directory to temp dir for test isolation |
| test/sanity/src/desktop.test.ts | Added universal binary support with ARCHPREFERENCE environment variable for x64 Mac; renamed test for clarity |
| test/sanity/src/context.ts | Added retry logic to fetchNoErrors; added webkit browser support for macOS; added helper methods for URL construction and random value generation; changed Mac entry point to Electron executable |
| test/sanity/package.json | Added postinstall script to install Playwright browsers |
Comments suppressed due to low confidence (2)
test/sanity/src/context.ts:421
- The removal of the quarantine attribute clearing (xattr -rd com.apple.quarantine) may cause macOS Gatekeeper to prevent the app from launching during tests. On macOS, downloaded applications are marked with a quarantine attribute that triggers security warnings or blocks execution. Unless this is being handled elsewhere or the test environment doesn't require it, this could cause test failures. Consider whether this removal is intentional and safe for the test environment.
This issue also appears in the following locations of the same file:
- line 397
const entryPoint = path.join(bundleDir, appName, 'Contents/MacOS/Electron');
if (!fs.existsSync(entryPoint)) {
this.error(`Desktop entry point does not exist: ${entryPoint}`);
}
this.log(`VS Code executable at: ${entryPoint}`);
return entryPoint;
test/sanity/src/context.ts:399
- The documentation comment is misleading. While the method now returns the Electron executable path (Contents/MacOS/Electron), the comment still says it "Prepares a macOS .app bundle for execution by removing the quarantine attribute" but the code no longer removes the quarantine attribute. Update the comment to accurately reflect what the method does now, which is simply locating and returning the Electron executable path.
* Prepares a macOS .app bundle for execution by removing the quarantine attribute.
* @param bundleDir The directory containing the .app bundle.
* @returns The path to the VS Code Electron executable.
bpasero
approved these changes
Jan 11, 2026
Member
|
@dmitrivMS just fyi, quite some Copilot comments. |
Contributor
Author
Will do a follow-up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR feedback from previous changes (using better UI waiting mechanisms, better error handling).
Fixes all issues with x64 MacOS - all tests pass now.
Improved reliability of download step with retries.
Contributes towards #279402