Fix E2E harness launch delegation and welcome-dialog interference - #244
Draft
Chiara Mooney (chiaramooney) wants to merge 3 commits into
Draft
Chiara Mooney (chiaramooney) wants to merge 3 commits into
Chiara Mooney (chiaramooney) wants to merge 3 commits into
Conversation
The Playwright E2E suite failed for 23 of its specs on developer machines. The cause was two-part, both in the shared launcher, and neither involved product code. 1. Launch delegation. helpers.ts launched VS Code without an isolated --user-data-dir. When a developer already has VS Code running (the common case), the newly spawned Code.exe hands its window off to the existing instance and exits immediately, so Playwright's app.firstWindow() never resolves and the launch times out. Because the 13 manifest-editor specs share one instance via shared-context.ts, a single launch failure cascaded into all of them. Every launch now gets its own --user-data-dir and --extensions-dir under a temp directory, removed on teardown. 2. Welcome dialog. An isolated profile is brand new, so VS Code shows its Welcome/Trust dialog on first start. That dialog holds focus and swallows the Ctrl+Shift+P used to open the Command Palette, which moved the failure from launch into getWebviewFrame. Launch now dismisses it tolerantly (no-op when no dialog appears). All launches go through one path, launchVSCodeApp() in helpers.ts; sign-quickpick.spec.ts and input-folder-validation.spec.ts no longer carry their own copies. Fixing the launch exposed latent races in the specs that the launch failure had been masking, fixed here with web-first assertions: - Command Palette interaction used fixed sleeps and a blind Enter. On a cold isolated profile the extension can still be activating, so the command was not yet listed. runCommand() now waits for the command row, and does not return until the palette has actually gone, since the quick-input widget is reused by whatever the command shows next. - The sign specs read placeholders with a non-retrying getAttribute and asserted toHaveCount(0) on rows that VS Code leaves in the DOM after dismissal. They now assert on placeholder attributes and widget visibility. - Focusing the manifest tab left a hover tooltip over the editor area that intercepted clicks inside the webview; the pointer is now moved away. Test results, run with a normal VS Code instance already running (the exact condition that triggers the bug): before: 0 passed, 23 failed, 172 did not run after: 192 passed, 0 failed, 4 flaky (pass on retry) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Self-review follow-up to the launch-delegation fix. Removed two changes that were not justified by the root cause: - openManifestEditor no longer clicks the manifest editor tab. That click was speculative (guarding against a Get Started tab stealing the active editor, which was never observed) and it raised a hover tooltip overlay that intercepted clicks inside the webview iframe -- a problem that then needed its own mouse.move/.hover-contents workaround. Removing the click removes the need for the workaround. - resetManifest reverted to its original form. The content-based wait added to de-flake the fixture-swap reload delivered no measured benefit across two full runs. Verified: 192 passed / 0 failed / 4 flaky, identical to the run with both changes present, with a normal VS Code instance running. Also: test:e2e now runs pm run compile first. The extension's main is ./dist/extension.js, built by esbuild. pretest only runs compile-tsc, which builds out/ (test-only). Without this, running the E2E suite against a stale or missing dist/ fails activation with "Cannot find module" and every test fails in a way that looks like a product bug. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
VSIX Build
Updated 2026-09-14 19:41:42 UTC · commit |
Chiara Mooney (chiaramooney)
marked this pull request as draft
September 14, 2026 21:42
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
23 of the E2E specs failed on developer machines. The cause was two-part, both in the shared Playwright launcher, and neither involved product code — every change here is under
src/test/e2e/, plus a one-linepackage.jsonscript fix.Before / after
Run on a developer machine with a normal VS Code instance already running, which is the exact condition that triggers the bug:
4 tests are flaky (fail once, pass on retry); details below.
npx tsc --noEmitis clean andnpm run lintholds at the 12 pre-existing warnings / 0 errors baseline.Root cause
1. Launch delegation — why an isolated
--user-data-diris requiredhelpers.tslaunched VS Code without--user-data-dir. When the developer already has VS Code running — the common case — the newly spawnedCode.exedoes not create its own window. It hands the request off to the existing instance and then exits immediately. Playwright is left holding a dead process, soapp.firstWindow()never resolves and the launch times out.This is not obvious from the failure output, which just looks like a slow start-up, so it is worth stating plainly for future readers: without an isolated user-data dir, the launch is silently delegated to another process and the test harness loses its window.
The blast radius was large because the 13 manifest-editor specs share a single VS Code instance via
shared-context.ts— one launch failure cascaded into all of them, which is why 172 tests never even ran.Every launch now gets its own
--user-data-dirand--extensions-dircreated under a temp directory (the latter also keeps the developer's installed extensions out of the run), removed on teardown.2. Welcome dialog — why dismissal is required
An isolated user-data dir means a brand-new profile, and a brand-new profile means VS Code shows its Welcome / Trust dialog on first start. That dialog holds keyboard focus and swallows the
Ctrl+Shift+Pused to open the Command Palette. Fixing only part 1 therefore just moves the failure from the launch intogetWebviewFrame.Launch now dismisses the dialog, tolerantly — it is a no-op when no dialog appears, so it is safe on any profile state.
Fixing both parts is what takes the suite green; either alone is not enough.
One launch path
All launches now go through
launchVSCodeApp()inhelpers.ts.sign-quickpick.spec.tsandinput-folder-validation.spec.tsno longer carry their own copies of the launch logic.test:e2enow buildsdist/The only change outside
src/test/e2e/:The extension's
mainis./dist/extension.js, built by esbuild.pretestonly runscompile-tsc, which buildsout/— that is test-only and does not make the extension loadable. Running the E2E suite against a stale or missingdist/fails activation with "Cannot find module", and every test then fails in a way that looks like a product bug rather than a build problem. Since this PR is about making the suite reliable, the trap belongs closed here.Latent spec races this exposed
With the launch fixed, the specs got far enough to reveal pre-existing races that the launch failure had been masking. Fixed here with web-first assertions rather than longer sleeps:
Enter. On a cold isolated profile the extension can still be activating, so the command was not yet listed andEnterdid nothing.runCommand()now waits for the command row to appear, and does not return until the palette has actually gone — the quick-input widget is reused by whatever the command shows next, so a caller that inspects it too early reads the dismissing palette instead of the command's own UI.getAttribute, and assertedtoHaveCount(0)on rows that VS Code leaves in the DOM after dismissal. They now assert on placeholder attributes and on widget visibility.Remaining flakes (not introduced here)
These fail at most once and pass on retry; all are pre-existing test-ordering / editor-reload races unrelated to launch:
applications-tab.spec.ts› Show Name on Tiles checkboxes are visible — the test does not re-select the Applications tab, so a preceding test's edit can leave the card unrendered.background-task-fixture.spec.ts/push-notifications-fixture.spec.ts› identity fields are populated correctly, andproperties-tab.spec.ts› logo path is populated — the custom editor occasionally does not reload on the fixture swap within the wait window.I tried a content-based wait for the last group and it produced no measured improvement across two full runs, so I reverted it rather than leave an unproven change in the diff. These look like they may be real editor-reload behavior worth a separate investigation, and I left them alone deliberately rather than papering over them.
Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com