Listen for added workspace folders to also testing.activateLegacyWorkspace() them#26012
Listen for added workspace folders to also testing.activateLegacyWorkspace() them#26012vaclavHala wants to merge 3 commits into
Conversation
…space() them so testing works there
There was a problem hiding this comment.
Pull request overview
This PR addresses #26011 by ensuring legacy testing adapters are initialized for workspace folders that are added after the Python extension has already activated, so test discovery works without requiring a VS Code restart.
Changes:
- In legacy mode, register a
onDidChangeWorkspaceFolderslistener and callactivateLegacyWorkspace()for newly added folders. - Add a unit test covering discovery in a workspace folder added post-activation.
Show a summary per file
| File | Description |
|---|---|
| src/client/testing/testController/controller.ts | Hooks workspace-folder additions in legacy mode to initialize per-workspace legacy test adapters. |
| src/test/testing/testController/controller.unit.test.ts | Adds coverage to ensure discovery works for folders added after activation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
| this.disposables.push( | ||
| this.workspaceService.onDidChangeWorkspaceFolders((evt) => { | ||
| evt.added.forEach((workspace) => { | ||
| this.activateLegacyWorkspace(workspace); | ||
| }); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
This will leave disposables created by this.activateLegacyWorkspace(workspace); un-disposed. It's probably not an issue since they are just file listeners for paths no longer in the workspace (since it just got removed), and the extra complexity needed to keep track of them so we can dispose them here may not be worth it. The disposables would eventually get disposed when the whole controller is disposed, so they aren't completely orphaned.
Do you want me to just apply this suggestion from copilot or should I also make sure all disposables created for this workspace folder are symmetrically disposed when the folder is removed?
There was a problem hiding this comment.
Also I don't like having the error assembled here like this
`DiscoveryError:${workspace.uri.fsPath}`
I'd prefer to have that done through some shared code which deals with how that ID works, but I'm not sure what it should be. I grepped through the code a bit, maybe this could work?
buildErrorNodeOptions(workspace.uri, "", "").idThen again looking through the code I can see many similar places where the ID is constructed inline as string, so maybe I should just do the same thing here after all?
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Fixes #26011