-
-
Notifications
You must be signed in to change notification settings - Fork 12
feat(vscode): support workspaces and monorepo #688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for rstest-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
a3b4066 to
a31eb0b
Compare
There was a problem hiding this 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 adds comprehensive support for workspaces and monorepo configurations to the rstest VS Code extension. It introduces a hierarchical project structure where each workspace can contain multiple rstest config files, enabling better support for multi-project setups.
Key changes:
- Introduced
valibotfor configuration schema validation with fallback values - Restructured extension to support multiple workspaces and config files per workspace
- Refactored worker initialization to pass config file path and use proper root directory
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Added valibot dependency for configuration validation |
| packages/vscode/package.json | Added configFileGlobPattern setting and valibot dependency; added watch:local script |
| packages/vscode/src/config.ts | Rewrote config management using valibot schema with new watchConfigValue function |
| packages/vscode/src/extension.ts | Refactored to support multiple workspaces with WorkspaceManager pattern |
| packages/vscode/src/master.ts | Updated RstestApi to accept workspace, root, and config file path in constructor |
| packages/vscode/src/project.ts | New file implementing WorkspaceManager and Project classes for hierarchical management |
| packages/vscode/src/testTree.ts | Updated TestFile and TestCase to store and use RstestApi reference |
| packages/vscode/src/types.ts | Changed WorkerInitData to use root and configFilePath instead of cwd |
| packages/vscode/src/utils.ts | Simplified to shouldIgnoreUri function, removed unused workspace pattern helpers |
| packages/vscode/src/worker/index.ts | Updated to accept and use configFilePath parameter |
| packages/vscode/tests/suite/config.test.ts | Updated tests to use new helper functions and refreshHandler |
| packages/vscode/tests/suite/helpers.ts | Added helper functions for test item collection |
| packages/vscode/tests/suite/index.test.ts | Updated to verify new workspace/project hierarchy |
| .vscode/launch.json | Changed preLaunchTask from build:local to watch:local |
| .vscode/settings.json | Added source.organizeImports: "never" setting |
| .vscode/tasks.json | Changed build task to watch task |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| token.onCancellationRequested(() => watcher.dispose()); | ||
| watcher.onDidCreate((file) => this.handleAddConfigFile(file)); | ||
| watcher.onDidDelete((file) => this.handleRemoveConfigFile(file)); | ||
| } |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File watchers created in the loop are not stored for cleanup. While they're disposed via the cancellation token, this approach makes it harder to track and manage resources. Consider storing watchers in an array for explicit cleanup.
packages/vscode/src/extension.ts
Outdated
| this.ctrl.refreshHandler = () => { | ||
| this.startScanWorkspaces(); | ||
| }; | ||
| this.ctrl.resolveHandler = (item) => { | ||
| if (!item) { | ||
| this.startScanWorkspaces(); | ||
| } | ||
| }; |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The startScanWorkspaces() method is called both in the constructor (line 33) and in refreshHandler (line 53) and resolveHandler (line 57). On refresh/resolve, this disposes all existing workspaces and recreates them, which could interrupt running tests. Consider implementing a more graceful update mechanism.
| // TODO catch and set error | ||
| this.api.createChildProcess(); | ||
|
|
||
| this.configValueWatcher = this.startWatchingWorkspace(); | ||
| } |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The api.createChildProcess() call at line 132 is not awaited. If this is an async operation that can fail, errors won't be properly handled. Consider awaiting it or adding proper error handling.
| // TODO catch and set error | |
| this.api.createChildProcess(); | |
| this.configValueWatcher = this.startWatchingWorkspace(); | |
| } | |
| this.configValueWatcher = this.startWatchingWorkspace(); | |
| } | |
| async init() { | |
| try { | |
| await this.api.createChildProcess(); | |
| } catch (err) { | |
| // TODO: handle error, e.g., log or set error state | |
| console.error('Failed to create child process:', err); | |
| } | |
| } |
|
|
||
| return value as ExtensionConfig[K]; | ||
| const value = vscode.workspace.getConfiguration('rstest', scope).get(key); | ||
| return v.parse(configSchema.entries[key], value) as ExtensionConfig[K]; |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of v.parse here may throw an error if the configuration value doesn't match the schema, which could crash the extension. Consider using v.safeParse instead and handling validation errors gracefully, or providing a fallback value.
| return v.parse(configSchema.entries[key], value) as ExtensionConfig[K]; | |
| const result = v.safeParse(configSchema.entries[key], value); | |
| if (result.success) { | |
| return result.output as ExtensionConfig[K]; | |
| } else { | |
| // If validation fails, use the fallback value if defined, otherwise undefined | |
| // v.fallback sets the default value, so we can parse 'undefined' to get it | |
| const fallbackResult = v.safeParse(configSchema.entries[key], undefined); | |
| if (fallbackResult.success) { | |
| return fallbackResult.output as ExtensionConfig[K]; | |
| } | |
| return undefined as ExtensionConfig[K]; | |
| } |
| // use this.childProcess to catch post is called after process killed | ||
| post: (data) => this.childProcess?.send(data), |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this.childProcess?.send(data) without checking the return value or handling errors may result in silent failures if the process has exited or the message cannot be sent. Consider checking the result of send() or listening for error events.
| this.handleRemoveWorkspace(removed); | ||
| } | ||
| }, | ||
| ); |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workspaceWatcher disposable is created but never disposed when the extension is deactivated. This could lead to resource leaks. Consider storing it in context.subscriptions or disposing it in a deactivation handler.
| ); | |
| ); | |
| this.context.subscriptions.push(this.workspaceWatcher); |
| for (const pattern of patterns) { | ||
| const watcher = vscode.workspace.createFileSystemWatcher( | ||
| pattern, | ||
| false, | ||
| false, | ||
| false, | ||
| ); | ||
|
|
||
| token.onCancellationRequested(() => watcher.dispose()); | ||
| watcher.onDidCreate((uri) => { | ||
| if (shouldIgnoreUri(uri)) return; | ||
| this.updateOrCreateFile(uri); | ||
| }); | ||
| watcher.onDidChange((uri) => { | ||
| if (shouldIgnoreUri(uri)) return; | ||
| this.updateOrCreateFile(uri); | ||
| }); | ||
| watcher.onDidDelete((uri) => { | ||
| this.projectTestItem.children.delete(uri.toString()); | ||
| }); | ||
| } |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the config watcher above, file watchers created in this loop are not explicitly tracked. If the config changes multiple times rapidly, old watchers may not be properly disposed before new ones are created.
| return Object.fromEntries( | ||
| Object.keys(configSchema.entries).map((key) => [ | ||
| key, | ||
| getConfigValue(key as keyof ExtensionConfig, scope), | ||
| ]), | ||
| ) as ExtensionConfig; |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Object.fromEntries with Object.keys loses type safety. The iteration could include unexpected keys if configSchema.entries is modified. Consider using a more explicit approach or using Object.entries directly on a typed object.
|
|
||
| public dispose() { | ||
| this.childProcess?.kill(); | ||
| this.childProcess = null; |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dispose() method kills the child process but doesn't wait for it to exit or clean up the worker reference. This could lead to attempts to use a disposed worker. Consider setting this.worker = null after killing the process.
| this.childProcess = null; | |
| this.childProcess = null; | |
| this.worker = null; |
fi3ework
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally LGTM, could you add some test case to ensure the new feature?
|
writing vscode test case is painful, I'm trying to implement run rstest inside vscode first. ❯ node tests/runTest.mts
✔ Validated version: 1.106.1
✔ Found existing install in /github/rstest/packages/vscode/.vscode-test/vscode-darwin-arm64-1.106.1
[main 2025-11-18T17:09:40.848Z] update#setState disabled
[main 2025-11-18T17:09:40.849Z] update#ctor - updates are disabled by the environment
ChatSessionStore: Migrating 0 chat sessions from storage service to file system
Started local extension host with pid 79338.
MCP Registry configured: https://api.mcp.github.com
Loading development extension at /github/rstest/packages/vscode
Settings Sync: Account status changed from uninitialized to unavailable
Test controller found with ID: rstest
Test controller has 1 root items
Root test item: file:///github/rstest/packages/vscode/tests/fixtures - fixtures
Child test item: file:///github/rstest/packages/vscode/tests/fixtures/rstest.config.ts - rstest.config.ts
✓ suite/index.test.ts (1)
✓ Extension Test Suite > Extension should discover test items (5.13s)
Test Files 1 passed
Tests 1 passed
Duration 5.21s (build 27ms, tests 5.18s)
[main 2025-11-18T17:09:47.494Z] Extension host with pid 79338 exited with code: 0, signal: unknown.
Exit code: 0 |
f795174 to
9a3680a
Compare
It seems impossible to achieve this at the moment, so I've given up. Test case is added. |
Summary
config.tsinspectwithget, which already picked value by prioritywatchConfigValuelaunch.json: change preLaunchTask from build to watchworker/index.ts: pass configFilePath to rstest core, renamecwdtorootextension.tsmaster.ts: using config file path as cwd when resolve and spawn workerproject.ts/WorkspaceManager: scan and watch all rstest config files matching glob inside this workspaceporject.ts/ProjecttestTree.tsrun test with corresponding workerRelated Links
Checklist