Skip to content
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

bugfix: display tests in explorer per workspace folder #1394

Merged
merged 1 commit into from May 22, 2023

Conversation

kasiaMarek
Copy link
Contributor

Previously: test suites were displayed per build target.
Now: we group them also per workspace folder

connected to: scalameta/metals#5212
resolves: scalameta/metals#5189

@kasiaMarek kasiaMarek force-pushed the multi-root-tests-explorer branch 2 times, most recently from 3d2e4e5 to 4d700ee Compare May 10, 2023 15:00
@kasiaMarek kasiaMarek requested a review from kpodsiad May 10, 2023 15:12
Copy link
Member

@kpodsiad kpodsiad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't run this branch myself, but changes looks reasonable. LGTM


export type MetalsTestItemKind =
| "workspaceFolder"
| "project"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if module wouldn't be a better name instead of project

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, "project" is confusing in this context. If anything a workspace folder should be named "project".

Comment on lines 76 to 82
if (
kind === "project" ||
kind === "package" ||
kind === "workspaceFolder"
) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have write some documentation about this :( Consider adding my loud thoughts as comments/proper doc somewhere.
there is no need to change more bits here because createRunQueue call itself recursively, so almost nothing changed in terms of running - still kind suite is the operating atom here and in result analyzer.

): vscode.TestItem {
const buildTarget = testController.items.get(targetName);
const buildTarget = workspaceFolderItem.children.get(targetName);
if (buildTarget) {
return buildTarget;
}

const createdNode = testController.createTestItem(targetName, targetName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should prefix target's test item id with workspace? Or it isn't necessary because id has to be unique only in a given subtree and if we're child of workspace then we're already guaranteed to be unique?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@param id Identifier for the TestItem. The test item's ID must be in the {@link TestItemCollection} it's added to
where
TestItemCollection is a collection of test items, found in TestItem.children and TestController.items.

So it only has to be unique among its siblings and we always access it by first accessing the parent (workspace folder).

@kasiaMarek kasiaMarek marked this pull request as ready for review May 12, 2023 08:16
@kasiaMarek kasiaMarek merged commit 4d3fd7e into scalameta:main May 22, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test explorer missing files with multiple folders and same names
2 participants