Skip to content

Conversation

@dmitrivMS
Copy link
Contributor

@dmitrivMS dmitrivMS commented Jan 11, 2026

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

Copilot AI review requested due to automatic review settings January 11, 2026 12:12
@dmitrivMS dmitrivMS enabled auto-merge January 11, 2026 12:12
@dmitrivMS dmitrivMS self-assigned this Jan 11, 2026
@dmitrivMS dmitrivMS added the engineering VS Code - Build / issue tracking / etc. label Jan 11, 2026
@dmitrivMS dmitrivMS modified the milestones: February 2026, January 2026 Jan 11, 2026
Copy link
Contributor

Copilot AI left a 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 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.

@dmitrivMS dmitrivMS merged commit 3bb929b into main Jan 11, 2026
27 of 28 checks passed
@dmitrivMS dmitrivMS deleted the dev/dmitriv/sanity-tests-fixes branch January 11, 2026 12:44
@bpasero
Copy link
Member

bpasero commented Jan 11, 2026

@dmitrivMS just fyi, quite some Copilot comments.

@dmitrivMS
Copy link
Contributor Author

@dmitrivMS just fyi, quite some Copilot comments.

Will do a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engineering VS Code - Build / issue tracking / etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants