🧪 add support for opening vscode in web environment #928
Conversation
Signed-off-by: Alejandro Brugarolas <[email protected]>
WalkthroughIntroduces a platform-abstracted VSCode E2E framework. The base VSCode page is refactored into an abstract class with command category centralization and new abstract methods for lifecycle, UI, settings, and cache handling. Adds concrete implementations for desktop (Electron) and web (Playwright browser), plus a factory that selects between them via WEB_ENV. Updates multiple tests and global setup to use the factory and new initialization methods. Adds environment variables and docs for web configuration, ignores a new web-state.json artifact, and assigns deterministic ids to specific webview UI elements used by tests. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/pages/vscode.page.ts (1)
101-103: Use expect().toBeEnabled() instead of isEnabled with timeoutLocator.isEnabled() does not wait; passing timeout is ignored. Use expect with timeout.
- await stopButton.isEnabled({ timeout: 180000 }); + await expect(stopButton).toBeEnabled({ timeout: 180000 });
🧹 Nitpick comments (9)
tests/.env.example (1)
28-35: Consider adding usage guidance for web configuration.The new WEB_* environment variables are documented but might benefit from a brief comment explaining when these should be used (e.g., "Set these when running tests against a web-based VSCode instance like DevSpaces").
Apply this diff to add clarifying context:
######################### ### WEB CONFIGURATION ### ######################### +# Set these variables when running tests in a web-based VSCode environment (e.g., DevSpaces) # WEB_ENV=1 # WEB_BASE_URL="https://devspaces.apps.com" # WEB_LOGIN="user" # WEB_PASSWORD="password"tests/e2e/tests/base/llm-revert-check.test.ts (1)
9-9: Factory migration looks good; address subfolder open for WEB_ENV
- Import/use of VSCodeFactory is fine.
- The second open uses a subfolder path; this won’t work in web/devspaces yet (TODO). Recommend adding a factory method (e.g., openSubpath) that, when WEB_ENV is set, navigates to subfolder via Explorer, rather than relying on a local path.
Also applies to: 35-40, 43-45
tests/e2e/pages/vscode-web.page.ts (3)
38-39: Replace fixed 120s sleep with deterministic waitsA hard wait increases flakiness and runtime. Prefer waiting for specific UI states (you already do for Trust and Java readiness).
Apply this diff:
- // TODO (abrugaro): Remove this and replace for an assertion - await newPage.waitForTimeout(120000); + // Wait for Restricted Mode banner to appear before managing trust + await expect( + newPage.getByLabel('Restricted Mode is intended').getByRole('button', { name: 'Manage' }).first() + ).toBeVisible({ timeout: 120000 });
3-8: Unify Playwright imports to avoid duplicate depsOptional: import chromium and BrowserContextOptions from @playwright/test to keep a single dependency surface when using the test runner.
Apply this diff:
-import { BrowserContextOptions } from 'playwright'; -import { expect, Page } from '@playwright/test'; +import { expect, Page, chromium, type BrowserContextOptions } from '@playwright/test'; -import { chromium } from 'playwright';
109-142: LLM cache paths likely ineffective in WEB_ENVThese unzip/copy paths target local FS (repoDir/.vscode/cache). In web/devspaces the workspace is remote, so this won’t populate the extension cache. Consider implementing remote file upload or a workspace command to receive cache data; otherwise skip in WEB_ENV.
tests/e2e/pages/vscode-desktop.page.ts (3)
91-96: Remove duplicate firstWindow callfirstWindow() is invoked twice; the first call result is ignored.
- await vscodeApp.firstWindow(); - const window = await vscodeApp.firstWindow({ timeout: 60000 }); console.log('VSCode opened'); - const vscode = new VSCodeDesktop(vscodeApp, window, repoDir); + const inferredRepoDir = + repoDir || + (repoUrl ? repoUrl.replace(/\/+$/, '').split('/').pop()?.replace(/\.git$/, '') || undefined : undefined); + const vscode = new VSCodeDesktop(vscodeApp, window, inferredRepoDir);
64-71: Validate executablePath exists (Linux fallback is brittle)On Linux you assume /usr/share/code/code; many distros use /usr/bin/code. Add an existence check and clearer error.
if (!executablePath) { if (getOSInfo() === 'linux') { - executablePath = '/usr/share/code/code'; + const candidates = ['/usr/share/code/code', '/usr/bin/code']; + executablePath = candidates.find((p) => fs.existsSync(p)); + if (!executablePath) { + throw new Error( + 'VSCode executable not found. Set VSCODE_EXECUTABLE_PATH or ensure code is in a known path.' + ); + } } else { throw new Error('VSCODE_EXECUTABLE_PATH env variable not provided'); } }
205-227: Nit: compute llmCachePaths() once to avoid repeated callsMinor readability/efficiency improvement.
public async ensureLLMCache(cleanup: boolean = false): Promise<void> { try { - const wspacePath = this.llmCachePaths().workspacePath; - const storedPath = this.llmCachePaths().storedPath; + const { workspacePath: wspacePath, storedPath } = this.llmCachePaths();tests/e2e/pages/vscode.page.ts (1)
270-274: Make selection clear step-by-stepChording Meta+a+Delete in one press can be flaky. Prefer sequential presses.
- await this.window.keyboard.press(`${modifier}+a+Delete`); + await this.window.keyboard.press(`${modifier}+a`); + await this.window.keyboard.press('Delete');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
tests/.env.example(1 hunks)tests/.gitignore(1 hunks)tests/docs/contrib/e2e-environment.md(1 hunks)tests/e2e/pages/vscode-desktop.page.ts(1 hunks)tests/e2e/pages/vscode-web.page.ts(1 hunks)tests/e2e/pages/vscode.page.ts(10 hunks)tests/e2e/tests/agent_flow_coolstore.test.ts(3 hunks)tests/e2e/tests/analyze_coolstore.test.ts(2 hunks)tests/e2e/tests/base/configure-and-run-analysis.test.ts(2 hunks)tests/e2e/tests/base/custom-binary-analysis.test.ts(2 hunks)tests/e2e/tests/base/fix-one-issue.test.ts(2 hunks)tests/e2e/tests/base/llm-revert-check.test.ts(2 hunks)tests/e2e/tests/solution-server/analysis-validation.test.ts(2 hunks)tests/e2e/tests/solution-server/partially-migrated-apps-scenario.test.ts(2 hunks)tests/e2e/utilities/vscode.factory.ts(1 hunks)tests/global.setup.ts(2 hunks)tests/playwright.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T16:16:56.005Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
Applied to files:
tests/e2e/tests/base/configure-and-run-analysis.test.tstests/global.setup.tstests/e2e/tests/base/fix-one-issue.test.ts
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
🧬 Code graph analysis (3)
tests/e2e/utilities/vscode.factory.ts (2)
tests/e2e/pages/vscode-desktop.page.ts (3)
open(26-103)VSCodeDesktop(15-268)init(111-122)tests/e2e/pages/vscode-web.page.ts (3)
open(19-49)VSCodeWeb(10-168)init(57-93)
tests/e2e/pages/vscode-desktop.page.ts (4)
tests/e2e/utilities/consts.ts (1)
TEST_DATA_DIR(4-4)tests/e2e/utilities/utils.ts (4)
cleanupRepo(35-57)writeOrUpdateSettingsJson(92-116)getOSInfo(20-33)extensionId(11-11)tests/e2e/utilities/vscode-commands.utils.ts (1)
installExtension(31-62)tests/e2e/utilities/archive.ts (2)
extractZip(6-8)createZip(10-29)
tests/e2e/pages/vscode-web.page.ts (1)
tests/e2e/utilities/archive.ts (2)
extractZip(6-8)createZip(10-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
🔇 Additional comments (12)
tests/.gitignore (1)
94-94: LGTM!The addition of
web-state.jsonto the ignore list is appropriate for excluding generated web authentication state artifacts from version control.tests/.env.example (1)
26-26: LGTM!Uncommenting
TEST_REPO_NAMEmakes the default test repository configuration more discoverable.tests/docs/contrib/e2e-environment.md (1)
76-82: LGTM!The documentation clearly describes the new web-based VSCode environment variables, including their requirements and when they should be used. The conditional requirement pattern (required only when
WEB_ENV=1) is well-documented.tests/playwright.config.ts (1)
47-50: Verify test dependencies for the devspaces project.The new
devspacesproject configuration looks correct. However, consider whether these tests should depend on thebaseproject (likeanalysis-testsdoes) if they require initial setup or extension installation.If devspaces tests require the extension to be installed and configured, add a dependency:
{ name: 'devspaces', testMatch: ['**/devspaces/**/*.test.ts'], + dependencies: ['base'], },tests/e2e/tests/solution-server/analysis-validation.test.ts (1)
13-13: LGTM!The migration to
VSCodeFactory.initis straightforward and maintains the existing test flow. The factory pattern enables environment-based routing between web and desktop VSCode implementations.Also applies to: 25-25
tests/e2e/tests/base/configure-and-run-analysis.test.ts (1)
9-9: LGTM!The migration to
VSCodeFactory.openis correct. Usingopen(rather thaninit) is appropriate here since this test suite manually handles profile creation and GenAI configuration in subsequent test steps.Also applies to: 26-26
tests/e2e/tests/base/custom-binary-analysis.test.ts (1)
9-9: LGTM!The migration to
VSCodeFactory.initis correct and maintains the test's flow. The subsequentcreateProfilecall is appropriate as this test creates a custom profile for binary analysis validation.Also applies to: 58-59
tests/e2e/tests/base/fix-one-issue.test.ts (1)
7-7: LGTM!The migration to
VSCodeFactory.initis correct. The test appropriately handles profile creation and GenAI configuration in subsequent steps, maintaining the existing test flow.Also applies to: 17-20
tests/e2e/tests/analyze_coolstore.test.ts (1)
11-11: LGTM: init via factory aligns with env‑aware routingSwitch to VSCodeFactory.init is consistent with the new pattern.
Also applies to: 27-27
tests/e2e/tests/agent_flow_coolstore.test.ts (1)
9-9: ```bash
#!/bin/bashInspect llmCachePaths in VSCodeWeb
rg -n "llmCachePaths" -C10 tests/e2e/pages/vscode-web.page.ts
Inspect llmCachePaths in VSCodeDesktop
rg -n "llmCachePaths" -C10 tests/e2e/pages/vscode-desktop.page.ts
</blockquote></details> <details> <summary>tests/e2e/pages/vscode.page.ts (2)</summary><blockquote> `426-429`: **Approve: robust click workaround** Using dispatchEvent('click') avoids overlay issues and aligns with past learnings. --- `68-79`: **Approve: centralized command category usage** Using VSCode.COMMAND_CATEGORY improves resilience when the view name changes and helps reuse across desktop/web. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
tests/e2e/tests/solution-server/partially-migrated-apps-scenario.test.ts
Show resolved
Hide resolved
Signed-off-by: Alejandro Brugarolas <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/pages/vscode.page.ts (1)
101-103: Playwright API misuse: isEnabled doesn’t accept timeout options.Replace isEnabled({ timeout }) with an assertion.
- await stopButton.isEnabled({ timeout: 180000 }); + await expect(stopButton).toBeEnabled({ timeout: 180000 });
♻️ Duplicate comments (1)
tests/e2e/pages/vscode.page.ts (1)
1-1: Fix return type and nullability in getView (contentFrame returns Frame | null).You import FrameLocator and declare getView(): Promise, but you return contentFrame() which resolves to Frame | null. Import Frame, change the signature to Promise, await contentFrame, and assert non-null.
-import { FrameLocator } from 'playwright'; +import type { Frame } from 'playwright';- public async getView(view: (typeof KAIViews)[keyof typeof KAIViews]): Promise<FrameLocator> { + public async getView(view: (typeof KAIViews)[keyof typeof KAIViews]): Promise<Frame> { @@ - if ((await innerFrameLocator.count()) === 1) { - return innerFrameLocator.contentFrame(); - } + if ((await innerFrameLocator.count()) === 1) { + const frame = await innerFrameLocator.contentFrame(); + if (!frame) throw new Error(`Iframe ${view} not ready`); + return frame; + }Also applies to: 247-264
🧹 Nitpick comments (3)
webview-ui/src/components/ViolationIncidentsList.tsx (1)
268-319: LGTM! Test IDs added for E2E testing support.The added
idattributes enable test selectors for the group-by dropdown and its options, which aligns with the PR's goal of supporting web environment testing. The IDs are descriptive, follow a consistent kebab-case naming pattern, and don't introduce any logic changes.Optional refinement: Consider using
data-testidattributes instead ofidattributes for test selectors. This is a more conventional practice because:
idattributes have functional significance (CSS specificity, ARIA relationships, anchor links) and must be unique across the entire pagedata-testidattributes are explicitly semantic for testing purposes and don't interfere with application behavior- Many testing frameworks (including Playwright) provide dedicated selectors for
data-testid(e.g.,page.getByTestId())Example refactor for the MenuToggle:
<MenuToggle - id="group-by-filter-dropdown" + data-testid="group-by-filter-dropdown" ref={toggleRef}And similarly for the SelectOption components (lines 301, 310, 319).
tests/e2e/pages/vscode.page.ts (2)
44-63: Avoid shadowing and standardize window access.Local const window shadows the DOM/global name and differs from this.window usage elsewhere. Rename locally to page and consider consistently using getWindow() or the window property throughout to avoid divergence.
- public async openLeftBarElement(name: string) { - const window = this.getWindow(); + public async openLeftBarElement(name: string) { + const page = this.getWindow(); @@ - const navLi = window.locator(`a[aria-label^="${name}"]`).locator('..'); + const navLi = page.locator(`a[aria-label^="${name}"]`).locator('..'); @@ - const moreButton = window.getByRole('button', { name: 'Additional Views' }).first(); + const moreButton = page.getByRole('button', { name: 'Additional Views' }).first(); @@ - const menuBtn = window.locator(`a.action-menu-item span[aria-label^="${name}"]`); + const menuBtn = page.locator(`a.action-menu-item span[aria-label^="${name}"]`);Additionally, consider removing either the abstract window property or getWindow() to have a single source of truth for the active Page.
31-42: Make command selection more robust in executeQuickCommand.The CSS+text engine chain can be brittle. Prefer role-based exact match.
// e.g. const option = this.window.getByRole('option', { name: new RegExp(`^${command.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}$`) }); await expect(option).toBeVisible(); await option.click();Also applies to: 37-39
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/e2e/pages/vscode-web.page.ts(1 hunks)tests/e2e/pages/vscode.page.ts(10 hunks)tests/e2e/utilities/vscode.factory.ts(1 hunks)webview-ui/src/components/ViolationIncidentsList.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/utilities/vscode.factory.ts
- tests/e2e/pages/vscode-web.page.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
🔇 Additional comments (2)
tests/e2e/pages/vscode.page.ts (2)
28-29: Resolved: pasteContent is now async.Signature updated to Promise, matching awaiting call sites.
426-431: Good: dispatchEvent('click') to bypass overlays.This matches the established workaround for notification overlays in constrained screens.
Based on learnings
Signed-off-by: Alejandro Brugarolas <[email protected]>
Signed-off-by: Alejandro Brugarolas <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/utilities/vscode.factory.ts (1)
24-28: Enhance JSDoc to describe function purpose.The JSDoc currently only lists parameters without describing what the function does or the role of the
WEB_ENVenvironment variable./** + * Factory function to initialize VSCode in either web or desktop mode based on WEB_ENV. + * Handles authentication and workspace setup before opening VSCode. * @param repoUrl * @param repoDir * @param branch + * @returns Promise resolving to an initialized VSCode instance */ export async function init(repoUrl?: string, repoDir?: string, branch?: string): Promise<VSCode> {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/pages/vscode-web.page.ts(1 hunks)tests/e2e/utilities/vscode.factory.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/pages/vscode-web.page.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/utilities/vscode.factory.ts (2)
tests/e2e/pages/vscode-web.page.ts (3)
open(20-58)VSCodeWeb(11-214)init(66-102)tests/e2e/pages/vscode-desktop.page.ts (3)
open(26-103)VSCodeDesktop(15-268)init(111-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
- GitHub Check: Build (windows)
1 participant
Part of #903
Current status
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation
Chores