Basic test explorer for VSCode based on testing API #14589

ShuiRuTian · 2023-04-17T01:34:27Z

Another implementation for #3601

The client side controls life-cycles of tests in this impl.

Another basic idea is to based on runnables to reuse existing logic.

NOTE: the names of many variables and API might be reconsidered.

You might want to read the "Known issue" section in the new added editors/code/src/test_explorer/README.md.
These issues might need to be fixed before the first release, waiting for opinions :)

Although we still lack some setting to control the behavior, the basic logic is done.

Behavior:
Bench, example and build script is not listed in the test explorer.

Features:

Basic VSCode test explorer feature

Test item tree and status update:

Note that, run will update the test item on the fly(as long as the output is logged), but debug will update the status when the debug session is over.

Test run history and error binding

Note that the error is bound to the item rahter than the line which occurs error.

Some special feature
3. test item flatten
This is the "normal" test item tree

Workspace - package - target - test modules - test
However, this might be kind of verbose, if you have

only one workspace, then maybe you do not want to click the only workspace to expand.
or only one package in the workspace,
or only one target in package

So we choose to flatten the test items, if there is only one workspace/package/target, we will ignore to build its corresponding test item.

In this picture, the target level is flattened.

And, if there is multiple workspaces/packages, but there is only one which has tests, we will not flatten it. but we will not show the ones which does not have tests. So, you might still see the result as if it's not flattened.

In this picture, there are two workspaces in fact, but the other does not have any tests.
So as if the workspace is not flattened.

However, this might be kind of confused, because it introduces inconsistent, and VSCode would not remember the last run result if the test item tree is changed.

Please leave your opinions whether the behavior is reasonable.

Issues:

Not support select multi items, like:

In this screenshot, user chooses two items, however, click run/debug button will get a notification like "Please only pick one to run/debug"
Because we load test items lazily now, when running a test suite, whose test cases are not loaded, their status is not updated. And if we load them later, it will be shown as if they are not executed:

crates/ide/src/test_items.rs

Veykril · 2023-04-27T13:36:23Z

editors/code/src/main.ts

+ * In most cases the work is finished by commands, and the context will pass itself
+ * as parameter to the command callback, which is defined when registering.
+ */
+export let raContext: Ctx | undefined;


We should not re-introduce this global, we had it before and intentionally removed it, as it makes it difficult to reason about.

Then everything should be done through commands, right?

it should go through commands, yes.

crates/rust-analyzer/src/handlers.rs

davidbarsky

Thanks for making this PR! I volunteered to Lukas to give this a first pass review as this is quite large and they're somewhat busy. Overall, I tried to leave comments here, but I have some questions and concerns:

The PR is rather large and I'm having difficulty understanding how a lot of the extension-side code is meant to work and interact with rust-analyzer-the-language-server. It seems to be reinventing a lot of functionality that already exists, but without the extensibility that already exists within rust-analyzer.
It's not clear to me if you're trying to do a global analysis to get all test targets or lazily update as tests update (or both). As best as I can tell, the client fetchs all crates, and whenever a new test is written, all displayed tests are removed and replaced. that doesn't feel like a good approach.
if I were to suggest an approach, i think the correct solution would be something similar to what Alex suggested: find all targets in the workspace, and for a given file, find the runnables. the explorer view can be updated as people run tests.

Ultimately, I'm not sure what direction you're interested in taking this, but I think an O(n) approach to test discovery should not be the approach, given that it doesn't work with lazy analysis.

I'll give some further comments in the future.

crates/ide/src/test_items.rs

davidbarsky · 2023-05-31T16:34:26Z

crates/project-model/src/cargo_workspace.rs

+    // sometimes it's useful to let the client know the project 
+    // structure.
+    // This property should only be used as JSON
+    pub origin_metadata: cargo_metadata::Metadata,


you shouldn't need to add Metadata here. if you really need to have the crate structure on the extension side of things—and I'm not sure you, given matklad's comment here—you only need to expose local packages and targets in the extension handler, which can be queried off the crate graph and salsa db.

I want to find the target based on BSP protocal, but matklad seems to(please correct me if I understand it wrongly!) suggest to use just the Cargo metadata. #14589 (comment)

crates/rust-analyzer/src/to_proto.rs

davidbarsky · 2023-05-31T18:03:04Z

crates/rust-analyzer/src/handlers.rs

+            if let ProjectWorkspace::Cargo { cargo, .. } = workspace {
+                Some(cargo.origin_metadata.clone())
+            } else {
+                None
+            }


While I agree with Alex that Buck (or related extensions) should handle setting up runnables, I think that they'd still benefit from getting a non-Cargo centric view of the immediate workspace.

(Don't treat this as a blocking nit/review.)

crates/rust-analyzer/src/lsp_ext.rs

editors/code/src/test_explorer/discover_and_update.ts

davidbarsky · 2023-05-31T19:00:29Z

editors/code/src/test_explorer/discover_and_update.ts

+        const targetUri = parentLocation[0].targetUri;
+        assert(targetUri.toLowerCase().endsWith("cargo.toml"));


the project manifest is not necessarily guaranteed to a Cargo.toml. I'd consider making use of the manifest path in #14931 if/when it lands.

davidbarsky · 2023-05-31T19:04:13Z

editors/code/src/test_explorer/discover_and_update.ts

+    // create target node
+    // This is necessary, because we do not know how many targets a package contains unless we fetch data of `cargo metadata`
+    // But we want to only fetch it when cargo file is changed, to make things more lazily.


I'm not sure if this is strictly necessary, as a change in the Cargo.toml will be picked up rust-analyzer-the-server, and querying the language server to get the new workspace structure feels like a far more scalable and reasonable solution than attempting to figure this out in the client.

It's only necessary for current implmentation.

We maintain a test model tree, and build test-item tree from test model tree.

For now, we only create test item for target when it does have test children. Which means, for target that not have test children, we remove it from our test model tree. So, when a new test is added to a file, the test might not have a valid target in the test model tree. We add the target node back to test model tree here.

Of course, we could choose to refactor the logic a bit, like, keep target nodes forever and change the logic how we rebuild test-item tree from test model tree.

editors/code/src/test_explorer/run_or_debug.ts

davidbarsky · 2023-05-31T19:08:58Z

editors/code/src/test_explorer/run_or_debug.ts

+    request: vscode.TestRunRequest,
+    token: vscode.CancellationToken
+) {
+    // TODO: Never run tests concurrently in client side.


it's not entirely clear to me as to why this limitation exists. if a user wants to run multiple tests, can the client not simply pass multiple arguments to the test runner?

This says, not allow user to click two "Run" button quickly, or , when a test is running, not allow another test is runed.

I am not sure what would happen if we allow this, because many process might download same thing meanwhile, or is there any lock file for test command.

And, in fact, we do not allow people run multi tests(but they could run tests for a package or for a whole test module) anyway for now. When user choose multi test-items and click run, we will log some message like "You could run one test item at one time."

davidbarsky

Thanks for making this PR! I volunteered to Lukas to give this a first pass review as this is quite large and they're somewhat busy. Overall, I tried to leave comments here, but I have some questions and concerns:

The PR is rather large and I'm having difficulty understanding how a lot of the extension-side code is meant to work and interact with rust-analyzer-the-language-server. It seems to be reinventing a lot of functionality that already exists, but without the extensibility that already exists within rust-analyzer.
It's not clear to me if you're trying to do a global analysis to get all test targets or lazily update as tests update (or both). As best as I can tell, the client fetchs all crates, and whenever a new test is written, all displayed tests are removed and replaced. that doesn't feel like a good approach.
if I were to suggest an approach, i think the correct solution would be something similar to what Alex suggested: find all targets in the workspace, and for a given file, find the runnables. the explorer view can be updated as people run tests.

Ultimately, I'm not sure what direction you're interested in taking this, but I think an O(n) approach to test discovery should not be the approach, given that it doesn't work with lazy analysis.

I'll give some further comments in the future.

ShuiRuTian · 2023-06-01T07:07:35Z

Thanks a lot for the comments!

Let me explain a bit at the high level how it works for now:

We maintain a test model tree, and build test-items(which is used by VSCode) tree by it.

At first time, we fetch workspaces, packages and targets form RA to initialize test model tree. Then we fetch runnables for target file. And because RA creates runnable for declaration moule(like "mod xxx;") if it contains tests, we know whether a declaration module contains tests. Then we get the definition file of the declaration module and fetch runnables for it. Repeat until there is no tests.

If it's not the first time, when a file is create/update/removed, we communicate with RA and update the test model tree.

And we rebuild the test-item tree everytime (this is kind of like red-green tree!), we do this because we try to flatten the test tree. We omit the workspace/package/target test-item if it only has one children.

It's kind of hard for me to understand whether this is O(n) for test discovery, because, when you find runnables for a file, you have to traversal some of its children in fact, to know whether a declaration module has tests or not.

ShuiRuTian · 2023-06-01T17:49:28Z

Maybe we also need think about whether we need to flatten test or not in which levels.

ShuiRuTian · 2023-06-13T05:59:55Z

The refactoring is finished 95%, and we fetch the children most lazily now.

PS: the only exception should be the direct children of targets. Because we do not want to show the target which does not have any tests. However, we get targets info through "cargo metadata", which will list all targets, so we fetch the children of targets eagerly to verify they have children or remove it if it does not.

bors · 2023-06-27T17:28:23Z

☔ The latest upstream changes (presumably #15151) made this pull request unmergeable. Please resolve the merge conflicts.

ShuiRuTian · 2023-07-11T14:46:47Z

Should be ready to be reviewed. Thanks for the patience :)

bors · 2023-07-13T16:29:12Z

☔ The latest upstream changes (presumably #15265) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2023-09-06T09:27:21Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

rustbot · 2023-09-12T14:21:01Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

45809b0

editors/code/src/test_explorer/README.md

ShuiRuTian · 2023-09-17T11:09:09Z

@Veykril Hi, may I request to review this PR again if you have time recently?

And if it's kind of too large, maybe we could have a meeting to talk about it on Zulip or Teams or something else.

Veykril · 2023-09-22T06:30:37Z

I'll try to take a look at this when I can but I can't make promises right now as to when that might be.

ShuiRuTian · 2023-09-22T08:13:18Z

@Veykril Sure. Anyway this is a OpenSource Project which is driven by interest, I also do not want it to be like a burden. Just at your own pace.

And please let me know what I could do to help this PR merged.

PS: I just found this PR might be part of @matklad's nefarious plan

bors · 2023-11-15T19:49:35Z

☔ The latest upstream changes (presumably #15902) made this pull request unmergeable. Please resolve the merge conflicts.

davidbarsky · 2024-02-06T18:24:52Z

Sorry for the delay in responding to this PR and letting it stagnate: I'm genuinely, really sorry about that. That being said, I think this feature is very useful, but I'm not sure there is sufficient expertise to maintain and understand the amount of Typescript that lives in this PR. This is not something rust-analyzer can reasonably take on at this time.

I think that this can be more successfully implemented as a standalone companion extension (reusing all the code that you wrote!), but might would require some additional, custom LSP extensions to rust-analyzer. We can work on figuring this out in a separate issue.

connor4312 · 2024-02-06T20:29:22Z

This is unfortunate, running tests in Rust is pretty painful in comparison to other languages with testing extensions available and this was something I was excited about. I hope a companion extension is viable 🙂

MartinBernstorff · 2024-02-07T08:08:36Z

Strong agree with @connor4312 here.

I completely understand your standpoint @davidbarsky. A 3000 line PR within a secondary language is quite a chunk.

I wonder whether this functionality might be sufficiently important to try and recruit maintainers for it? That said, I might be over-focusing on VSCode 😉

connor4312 · 2024-02-07T16:58:39Z

I would be happy to contribute, but I can't guarantee the time it deserves to be the owner of the repo/extension

ShuiRuTian · 2024-02-22T12:27:34Z

Sorry for the late response, I was on Chinese New Year Vacation.

@davidbarsky Hi, some questions:

This feature needs RA's functions, and personally, I think it's coupled closely. I am not sure, but it might make things harder if they are different program. RA needs to provide many events. Java's test extension is an example, it reuses the functions provided by Java's LSP. But I doubt whether RA should do the same thing. Extension/Plugin for a compiled language is always not that easy.
I could also help to maintain this feature. How could I prove it? Fix some other issues?

Or, maybe at least we could provide a switch flag(And we need to tell others it's experimental), which is turned of by default. So that someone could try this feature and provide feedback.

Add test explorer This PR implements the vscode testing api similar to #14589, this time using a set of lsp extensions in order to make it useful for clients other than vscode, and make the vscode client side logic simpler (its now around ~100 line of TS code) Fix #3601

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 17, 2023

ShuiRuTian changed the title ~~Basic test explorer for VSCode based on testing API~~ [WIP]Basic test explorer for VSCode based on testing API Apr 17, 2023

ShuiRuTian changed the title ~~[WIP]Basic test explorer for VSCode based on testing API~~ [BETA]Basic test explorer for VSCode based on testing API Apr 22, 2023

ShuiRuTian mentioned this pull request Apr 22, 2023

Support VS Code testing API #3601

Closed

Veykril self-assigned this Apr 22, 2023

Veykril reviewed May 3, 2023

View reviewed changes

Veykril removed their assignment May 3, 2023

davidbarsky reviewed May 31, 2023

View reviewed changes

ShuiRuTian force-pushed the basic-test-explorer branch from ee6ae8d to 6ee2ebe Compare June 1, 2023 15:25

ShuiRuTian mentioned this pull request Jun 20, 2023

[Rust] no stderr stream vadimcn/codelldb#948

Closed

ShuiRuTian requested review from Veykril, coder0xff and davidbarsky July 10, 2023 17:13

ShuiRuTian changed the title ~~[BETA]Basic test explorer for VSCode based on testing API~~ Basic test explorer for VSCode based on testing API Jul 11, 2023

rustbot added has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 6, 2023

king-11 reviewed Sep 15, 2023

View reviewed changes

editors/code/src/test_explorer/README.md Show resolved Hide resolved

king-11 reviewed Sep 15, 2023

View reviewed changes

editors/code/src/test_explorer/README.md Show resolved Hide resolved

king-11 reviewed Sep 15, 2023

View reviewed changes

editors/code/src/test_explorer/README.md Outdated Show resolved Hide resolved

king-11 reviewed Sep 15, 2023

View reviewed changes

editors/code/src/test_explorer/README.md Outdated Show resolved Hide resolved

ShuiRuTian force-pushed the basic-test-explorer branch from 98eb446 to 9fe4af6 Compare September 17, 2023 09:46

zaneduffield mentioned this pull request Sep 19, 2023

Add Rust Test Adapter swellaby/vscode-rust-pack#9

Open

ShuiRuTian force-pushed the basic-test-explorer branch from ab8acbe to 62db8f6 Compare September 21, 2023 07:50

Implement test explorer

bcd34c8

ShuiRuTian force-pushed the basic-test-explorer branch from 62db8f6 to bcd34c8 Compare September 21, 2023 08:13

davidbarsky closed this Feb 6, 2024

HKalbasi mentioned this pull request Feb 8, 2024

RIIR vscode extension #16515

Closed

HKalbasi mentioned this pull request Feb 24, 2024

feat: Add test explorer #16662

Merged

ShuiRuTian mentioned this pull request Mar 13, 2024

Enhance Test Explorer #16827

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic test explorer for VSCode based on testing API #14589

Basic test explorer for VSCode based on testing API #14589

ShuiRuTian commented Apr 17, 2023 •

edited

Loading

Veykril Apr 27, 2023

ShuiRuTian May 4, 2023

davidbarsky May 31, 2023

davidbarsky left a comment

davidbarsky May 31, 2023

ShuiRuTian Jun 1, 2023

davidbarsky May 31, 2023

davidbarsky May 31, 2023

davidbarsky May 31, 2023

ShuiRuTian Jun 1, 2023

davidbarsky May 31, 2023

ShuiRuTian Jun 1, 2023

davidbarsky left a comment

ShuiRuTian commented Jun 1, 2023 •

edited

Loading

ShuiRuTian commented Jun 1, 2023

ShuiRuTian commented Jun 13, 2023

bors commented Jun 27, 2023

ShuiRuTian commented Jul 11, 2023

bors commented Jul 13, 2023

rustbot commented Sep 6, 2023

rustbot commented Sep 12, 2023

ShuiRuTian commented Sep 17, 2023 •

edited

Loading

Veykril commented Sep 22, 2023

ShuiRuTian commented Sep 22, 2023

bors commented Nov 15, 2023

davidbarsky commented Feb 6, 2024

connor4312 commented Feb 6, 2024

MartinBernstorff commented Feb 7, 2024

connor4312 commented Feb 7, 2024

ShuiRuTian commented Feb 22, 2024 •

edited

Loading

		const targetUri = parentLocation[0].targetUri;
		assert(targetUri.toLowerCase().endsWith("cargo.toml"));

Basic test explorer for VSCode based on testing API #14589

Basic test explorer for VSCode based on testing API #14589

Conversation

ShuiRuTian commented Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbarsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbarsky left a comment

Choose a reason for hiding this comment

ShuiRuTian commented Jun 1, 2023 • edited Loading

ShuiRuTian commented Jun 1, 2023

ShuiRuTian commented Jun 13, 2023

bors commented Jun 27, 2023

ShuiRuTian commented Jul 11, 2023

bors commented Jul 13, 2023

rustbot commented Sep 6, 2023

rustbot commented Sep 12, 2023

ShuiRuTian commented Sep 17, 2023 • edited Loading

Veykril commented Sep 22, 2023

ShuiRuTian commented Sep 22, 2023

bors commented Nov 15, 2023

davidbarsky commented Feb 6, 2024

connor4312 commented Feb 6, 2024

MartinBernstorff commented Feb 7, 2024

connor4312 commented Feb 7, 2024

ShuiRuTian commented Feb 22, 2024 • edited Loading

ShuiRuTian commented Apr 17, 2023 •

edited

Loading

ShuiRuTian commented Jun 1, 2023 •

edited

Loading

ShuiRuTian commented Sep 17, 2023 •

edited

Loading

ShuiRuTian commented Feb 22, 2024 •

edited

Loading