Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end testing: a new GitHub Actions Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant GH as GitHub Actions
participant CI as ci job
participant E2E as e2e job
participant Runner as `@vscode/test-electron`
participant VSCode as VS Code (Electron)
participant Suite as Mocha Suite
PR->>GH: trigger pull-request workflow
GH->>CI: run ci job (install, build)
CI-->>GH: build completed
GH->>E2E: start e2e job (depends on ci)
E2E->>Runner: prepare temp workspace & copy fixtures
E2E->>Runner: launch VS Code with extension and workspace (xvfb-run)
Runner->>VSCode: start Electron with extension loaded
VSCode->>Suite: run Mocha E2E tests (activate, open files, invoke commands)
Suite->>VSCode: inspect webview/tabs and report results
Suite-->>Runner: tests finished
Runner->>E2E: cleanup temp workspace
E2E-->>GH: report job results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
=======================================
Coverage 93.68% 93.68%
=======================================
Files 8 8
Lines 269 269
Branches 101 101
=======================================
Hits 252 252
Misses 1 1
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/e2e/suite/index.ts (1)
12-17: Consider recursive test file discovery.The current implementation only discovers
*.test.jsfiles in the immediate__dirnamedirectory. If tests are organized in subdirectories in the future, they won't be discovered.This is fine for the current structure but may need adjustment as the test suite grows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/e2e/suite/index.ts` around lines 12 - 17, The current test discovery only collects files in testsRoot using readdirSync and misses nested test files; update the discovery to recursively walk testsRoot (or use a glob like fast-glob) to gather all files matching *.test.js under subdirectories, then call mocha.addFile for each discovered path (references: testsRoot, files, mocha.addFile). Implement a recursive helper that uses fs.readdirSync with Dirent (or a glob) to descend into directories and push matching filenames so future nested test suites are discovered.src/test/e2e/suite/extension.test.ts (1)
8-9: Consider failing fast if environment variables are missing.The empty string fallback for
E2E_WORKSPACE_DIRandE2E_FIXTURES_DIRcould lead to confusing test failures if the runner doesn't set them. An early assertion would make debugging easier.💡 Suggested improvement
-const workspaceDir = process.env.E2E_WORKSPACE_DIR ?? ''; -const fixturesDir = process.env.E2E_FIXTURES_DIR ?? ''; +const workspaceDir = process.env.E2E_WORKSPACE_DIR; +const fixturesDir = process.env.E2E_FIXTURES_DIR; + +if (!workspaceDir || !fixturesDir) { + throw new Error('E2E_WORKSPACE_DIR and E2E_FIXTURES_DIR environment variables must be set'); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/e2e/suite/extension.test.ts` around lines 8 - 9, The test currently falls back to empty strings for workspaceDir and fixturesDir which can mask missing environment configuration; change the top-level setup in extension.test.ts to immediately fail when process.env.E2E_WORKSPACE_DIR or process.env.E2E_FIXTURES_DIR are missing by checking workspaceDir and fixturesDir and throwing an informative error (or using your test framework's assertion) that names the missing variable(s) so the runner fails fast and points to the misconfiguration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/e2e/runTest.ts`:
- Line 17: The extensionDevelopmentPath is currently resolving four parent
levels from __dirname which, after compilation (out/test/e2e), walks outside the
project root; update the path resolution in runTest.ts (the
extensionDevelopmentPath constant) to traverse three parent directories instead
of four (i.e., use path.resolve(__dirname, '..', '..', '..') ) so it correctly
points to the project root where the compiled extension lives.
---
Nitpick comments:
In `@src/test/e2e/suite/extension.test.ts`:
- Around line 8-9: The test currently falls back to empty strings for
workspaceDir and fixturesDir which can mask missing environment configuration;
change the top-level setup in extension.test.ts to immediately fail when
process.env.E2E_WORKSPACE_DIR or process.env.E2E_FIXTURES_DIR are missing by
checking workspaceDir and fixturesDir and throwing an informative error (or
using your test framework's assertion) that names the missing variable(s) so the
runner fails fast and points to the misconfiguration.
In `@src/test/e2e/suite/index.ts`:
- Around line 12-17: The current test discovery only collects files in testsRoot
using readdirSync and misses nested test files; update the discovery to
recursively walk testsRoot (or use a glob like fast-glob) to gather all files
matching *.test.js under subdirectories, then call mocha.addFile for each
discovered path (references: testsRoot, files, mocha.addFile). Implement a
recursive helper that uses fs.readdirSync with Dirent (or a glob) to descend
into directories and push matching filenames so future nested test suites are
discovered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 453e4fbe-47fa-4fb1-a37a-a0b88c6b2042
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.github/workflows/pull-request.ymlpackage.jsonsrc/test/e2e/fixtures/sample.ymlsrc/test/e2e/runTest.tssrc/test/e2e/suite/extension.test.tssrc/test/e2e/suite/index.tstsconfig.jsonvitest.config.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/e2e/suite/extension.test.ts`:
- Around line 10-12: The tests use a fixed sleep helper named wait(ms) which
causes flaky E2E failures; replace it with a polling-based helper (e.g.,
waitForCondition or waitForTab) that repeatedly checks a predicate (such as
presence of a tab/title via the same API your tests use) at short intervals
until a timeout instead of sleeping a fixed duration. Update calls that
currently use wait(...) (notably the assertions around opening tabs in
extension.test.ts) to call waitForCondition(() => { /* check tab exists/active
*/ }, { timeout: 5000, interval: 100 }) and fail if the predicate never becomes
true; keep the original wait as deprecated or remove it after replacing all
usages.
- Around line 7-8: Add a fail-fast assertion in the test suite setup to ensure
E2E env vars are set: check that workspaceDir and fixturesDir (and the other
env-derived vars declared around lines 15-19) are non-empty in the suiteSetup
function, and throw or call assert.fail with a clear message if any are missing
so tests stop immediately and do not operate on unintended relative paths;
update suiteSetup to validate these variables before any test files or fixtures
are used.
- Around line 14-15: Add a note to the testing docs (03-testing.mdc) explicitly
exempting E2E tests that run with Mocha/@vscode/test-electron by clarifying that
files under the E2E glob (src/test/e2e/**/*.test.ts) are run via the separate
test:e2e command and not by the general test runner; reference the existing E2E
suite symbol (suite('Workflow Editor E2E', ...) and its suiteSetup usage) as an
example so maintainers understand the exception and update the .cursor/rules
glob guidance to exclude or document that E2E tests are handled differently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7e71ae3-0d32-4e4b-9134-23438b3fc028
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.jsonsrc/test/e2e/runTest.tssrc/test/e2e/suite/extension.test.tssrc/test/e2e/suite/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/e2e/suite/index.ts
- src/test/e2e/runTest.ts
- package.json
Comment on lines
+10
to
+12
| function wait(ms: number): Promise<void> { | ||
| return new Promise((resolve) => setTimeout(resolve, ms)); | ||
| } |
There was a problem hiding this comment.
Replace fixed sleeps with condition-based waits to reduce E2E flakiness.
Line 33/57/89 (and other fixed delays) rely on timing assumptions and can intermittently fail on slower CI runners. Prefer polling until the expected tab appears (with timeout).
Suggested fix
-function wait(ms: number): Promise<void> {
- return new Promise((resolve) => setTimeout(resolve, ms));
-}
+function wait(ms: number): Promise<void> {
+ return new Promise((resolve) => setTimeout(resolve, ms));
+}
+
+async function waitFor(condition: () => boolean, timeoutMs = 10000, intervalMs = 100): Promise<void> {
+ const start = Date.now();
+ while (Date.now() - start < timeoutMs) {
+ if (condition()) return;
+ await wait(intervalMs);
+ }
+ throw new Error(`Timed out after ${timeoutMs}ms`);
+}- await wait(2000);
+ await waitFor(() =>
+ vscode.window.tabGroups.all
+ .flatMap((g) => g.tabs)
+ .some((tab) => tab.input instanceof vscode.TabInputWebview && tab.label === 'sample.yml')
+ );Also applies to: 18-19, 24-25, 33-34, 57-58, 82-83, 89-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/e2e/suite/extension.test.ts` around lines 10 - 12, The tests use a
fixed sleep helper named wait(ms) which causes flaky E2E failures; replace it
with a polling-based helper (e.g., waitForCondition or waitForTab) that
repeatedly checks a predicate (such as presence of a tab/title via the same API
your tests use) at short intervals until a timeout instead of sleeping a fixed
duration. Update calls that currently use wait(...) (notably the assertions
around opening tabs in extension.test.ts) to call waitForCondition(() => { /*
check tab exists/active */ }, { timeout: 5000, interval: 100 }) and fail if the
predicate never becomes true; keep the original wait as deprecated or remove it
after replacing all usages.
Comment on lines
+14
to
+15
| suite('Workflow Editor E2E', () => { | ||
| suiteSetup(async () => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual content of the E2E test file
head -n 30 src/test/e2e/suite/extension.test.tsRepository: timoa/workflow-editor
Length of output: 1140
🏁 Script executed:
# Check for test framework configuration in package.json
rg -A 5 -B 5 '"test"|"e2e"' package.jsonRepository: timoa/workflow-editor
Length of output: 461
🏁 Script executed:
# Search for vitest and mocha configuration files
fd -e "config.ts" -e "config.js" | head -20Repository: timoa/workflow-editor
Length of output: 133
🏁 Script executed:
# Check what test framework is being used across src/
rg "suite\(|test\(|describe\(|it\(" src/ | head -20Repository: timoa/workflow-editor
Length of output: 1789
🏁 Script executed:
# Check package.json for `@vscode/test-electron` dependency
rg '@vscode/test-electron|mocha' package.jsonRepository: timoa/workflow-editor
Length of output: 157
🏁 Script executed:
# Check for any documentation about E2E tests
find . -name "*.md" -o -name "*.mdc" | xargs rg -l "e2e|E2E" 2>/dev/null | head -10Repository: timoa/workflow-editor
Length of output: 79
🏁 Script executed:
# Look at the E2E runner script
cat src/test/e2e/runTest.js 2>/dev/null || cat out/test/e2e/runTest.ts 2>/dev/null || find . -name "runTest.*" -type fRepository: timoa/workflow-editor
Length of output: 90
🏁 Script executed:
# Check cursor rules for any E2E exception documentation
cat .cursor/rules/*.mdc 2>/dev/null | grep -A 10 -B 2 -i "e2e\|mocha"Repository: timoa/workflow-editor
Length of output: 47
🏁 Script executed:
# Read the runTest.ts file to understand the E2E setup
cat src/test/e2e/runTest.tsRepository: timoa/workflow-editor
Length of output: 1708
🏁 Script executed:
# Check if there are any comments in cursor rules about E2E or test framework distinctions
cat .cursor/rules/*.mdc | head -100Repository: timoa/workflow-editor
Length of output: 5168
E2E tests correctly use Mocha via @vscode/test-electron, but the exception should be explicitly documented.
This file appropriately uses Mocha with @vscode/test-electron for VS Code extension E2E testing—this is the standard pattern for extension test runners and cannot use Vitest. However, the .cursor/rules files use broad globs (src/**/*.test.{ts,tsx}) that technically match this path without acknowledging the E2E exception. Add a clarification to 03-testing.mdc noting that src/test/e2e/**/*.test.ts is exempt and runs via the separate test:e2e command with Mocha/test-electron.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/e2e/suite/extension.test.ts` around lines 14 - 15, Add a note to the
testing docs (03-testing.mdc) explicitly exempting E2E tests that run with
Mocha/@vscode/test-electron by clarifying that files under the E2E glob
(src/test/e2e/**/*.test.ts) are run via the separate test:e2e command and not by
the general test runner; reference the existing E2E suite symbol
(suite('Workflow Editor E2E', ...) and its suiteSetup usage) as an example so
maintainers understand the exception and update the .cursor/rules glob guidance
to exclude or document that E2E tests are handled differently.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
94-98: Consider addingtimeout-minutesfor the E2E job.E2E tests involving VS Code can occasionally hang (e.g., waiting for UI elements). Without an explicit timeout, the job defaults to 6 hours. Adding a reasonable timeout improves reliability and prevents CI queue blockage.
⏱️ Suggested improvement
e2e: name: E2E Tests runs-on: ubuntu-latest needs: ci + timeout-minutes: 30 steps:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pull-request.yml around lines 94 - 98, The E2E job named "e2e" currently has no timeout, which can let VS Code UI tests hang and consume CI resources; add a timeout-minutes field to the e2e job definition (e.g., timeout-minutes: 60 or another team-agreed value) so the job is forcibly canceled after that period, placing the new key directly under the e2e job mapping alongside name and runs-on.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/pull-request.yml:
- Around line 94-98: The E2E job named "e2e" currently has no timeout, which can
let VS Code UI tests hang and consume CI resources; add a timeout-minutes field
to the e2e job definition (e.g., timeout-minutes: 60 or another team-agreed
value) so the job is forcibly canceled after that period, placing the new key
directly under the e2e job mapping alongside name and runs-on.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0d2ec5f-078e-4379-b9c3-f86ca9a955d7
📒 Files selected for processing (1)
.github/workflows/pull-request.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/e2e/suite/index.ts (1)
29-31: Make test execution order deterministic.
collectTestFiles()relies on directory iteration order, which can vary across environments and introduce flaky, order-dependent runs. Sort file paths before registering them with Mocha.Suggested patch
- for (const file of collectTestFiles(testsRoot)) { + const testFiles = collectTestFiles(testsRoot).sort((a, b) => a.localeCompare(b)); + for (const file of testFiles) { mocha.addFile(file); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/e2e/suite/index.ts` around lines 29 - 31, collectTestFiles(testsRoot) yields non-deterministic iteration order; make test registration deterministic by sorting the file paths before calling mocha.addFile. Replace the direct loop over collectTestFiles(testsRoot) with iterating over a sorted list (e.g., convert the iterable/array returned by collectTestFiles into an array and sort it) and then call mocha.addFile for each entry so test execution order is stable; reference collectTestFiles, testsRoot and mocha.addFile when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/e2e/suite/index.ts`:
- Around line 29-31: collectTestFiles(testsRoot) yields non-deterministic
iteration order; make test registration deterministic by sorting the file paths
before calling mocha.addFile. Replace the direct loop over
collectTestFiles(testsRoot) with iterating over a sorted list (e.g., convert the
iterable/array returned by collectTestFiles into an array and sort it) and then
call mocha.addFile for each entry so test execution order is stable; reference
collectTestFiles, testsRoot and mocha.addFile when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41909248-4f3c-4eb0-8a9b-66df47118d94
📒 Files selected for processing (2)
src/test/e2e/suite/extension.test.tssrc/test/e2e/suite/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/e2e/suite/extension.test.ts
🩺 React Doctor |
|
🎉 This PR is included in version 1.2.37 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
1 participant
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.
Description
Add the E2E tests suite + the first three tests:
Type of change
Checklist
Code quality
feat:,fix:,docs:,chore:)anyTypeScript types without justificationconsole.log, or commented-out blocksTesting
F5in VSCode to launch the Extension Development Hostpnpm test, and all tests passpnpm lint, and there are no lint errorsBuild & compatibility
pnpm run compileandpnpm run webpackwithout errorsDocumentation
README.mdif my change adds a new feature, keyboard shortcut, or changes existing behaviourScreenshots/recordings
Summary by CodeRabbit
Tests
Chores