feat: Add test explorer #16662

HKalbasi · 2024-02-24T20:39:35Z

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

HKalbasi · 2024-02-24T21:11:25Z

@ShuiRuTian Can you please review this? You can probably detect its problems since you have implemented this functionality one time :)

ShuiRuTian · 2024-02-26T06:36:06Z

Hi @HKalbasi Thanks for the ping.

I have not reviewed the details of this PR, and I even forgot some details about my PR, so the questions might be stupid.

Client side or server side, it's a problem. It's indeed arguable.
- I did the heavy job on client side based on this comment Support VS Code testing API #3601 (comment). It's also easy to use the rich feature of the client(VSCode).
- It's also fine to do the work on server side, BSP is a good example.
Some flaws:
- No debug support.
- No cancel support.
- No refresh support.
- No enqueued status in front end
Is it totally different with Runnables?
is test_id a crate? Does this means it always get all tests in a crate and send them to update?

I would try to pick some time to review this PR in detail.

HKalbasi · 2024-02-26T22:18:52Z

@ShuiRuTian thanks for your comment.

Client side or server side, it's a problem. It's indeed arguable.

My understanding from #16515 was that your PR was closed since it was heavy on the client side, which is not desired. Specially about the #3601 (comment) the part that this PR differs from it is that it runs the cargo test on the server side. The benefit of doing this is that otherwise n clients should each implement a cargo specific logic, but now they can use a regular lsp interface.

No debug support.

I intentionally deferred it to the future. Running the debug on the server is challenging (but is possible) and running it on the client is against the point of this PR, and test explorer is very useful even without debugging (one can double click on the test in the test explorer, and then use the old debug lens) so I ignored it for now.

No cancel support.

I added it in my last commit.

No refresh support.

I don't know exactly what is this. If you hint me about it, I can try to implement it.

No enqueued status in front end

I don't know much about this one either. When should it be called, and what is its effect in the UI?

Is it totally different with Runnables?

It doesn't reuse the runnables lsp extension, to keep the client side logic simpler. The runnables lsp extension is not a holy thing to keep it as is, and I think a polished version of this new extension set can replace (most of) the runnables extension in future when it gains support from more clients.

Tangentially, I think in some point we can retire the code lenses for run and debugging tests in favor of the client native UI.

is test_id a crate? Does this means it always get all tests in a crate and send them to update?

At the protocol level, it can be any module, and client side implementation can handle that, but the server side implementation currently work at the crate level, and return all tests in the crates of the open files and for resolve request. It is not hard to fix it, but it works well enough on the r-a repo so I don't think it is a problem and we can always fix it if it becomes one.

I would try to pick some time to review this PR in detail.

Thank you!

ShuiRuTian · 2024-02-27T08:28:05Z

Oops, my bad, I forgot to explain some terms.

No refresh support

This is a "refresh" icon. TestController has a method called "refreshHandler", which would be called. This feature is useful when something goes wrong(according to my experience, mainly because sync issues). We could rebuild everything.

No enqueued status in front end

This means the tests user chooses to run, but they might not be started yet. Just call them immediately when user click "run" button. TestRun has enqueued method. To know the status of tests, you could run command with cargo test -Z unstable-options --format=json (you might also find that --show-output and --report-time are helpful) and analytics the output events.

Tangentially, I think in some point we can retire the code lenses for run and debugging tests in favor of the client native UI.

Agree, but as you said, it might be not easy to add Debug support. And, according to my experience, again, it might need totally refactor when you need some new features. I reimplemented the whole world once.

Also, I am kind of curious what it looks like now.

I implemented some feature to try to make it look more naturally:

some pretty icons:

And if it's not a workspace, we only display unit tests and integration tests, the crate level are totally ignored.

HKalbasi · 2024-03-01T10:20:41Z

I added some icons, basic debug support, and refresh handler.

About enqueued status, my cargo only emits the started event. Do you mean to emit enqueued in that situation? Or there is some other events gated behind a flag (I tried --show-output and it didn't add enqueued events).

And if it's not a workspace, we only display unit tests and integration tests, the crate level are totally ignored.

I omitted this one for the simplicity of the implementation, but it can be easily added in a separate PR.

bors · 2024-03-02T09:52:19Z

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

HKalbasi · 2024-03-05T20:49:19Z

I'm going to merge this, I think this is in the scope of the project, since any other extensions implementing this functionality needs to either ask cargo or r-a to provide it, both are not ideal, and this PR is small enough, specially client side, to be included in the main extension. And this PR is certainly not perfect, but we can incrementally improve it and we shouldn't let perfect becomes the enemy of good.

@bors r+

bors · 2024-03-05T20:49:22Z

📌 Commit 44be243 has been approved by HKalbasi

It is now in the queue for this repository.

bors · 2024-03-05T20:50:29Z

⌛ Testing commit 44be243 with merge 767d5d3...

bors · 2024-03-05T21:02:15Z

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 767d5d3 to master...

nemethf

I've only read the documentation, so my "review" is not well informed. Sorry.

nemethf · 2024-03-05T21:37:02Z

docs/dev/lsp-extensions.md

+    range?: lc.Range | undefined;
+    // A human readable name for this test
+    label: string;
+    icon: "package" | "module" | "test";


Calling this kind would be more in line with the LSP specification. A client might want to something else with this information than displaying it as an icon.

For example, see the definition of CompletionItem

Fixed in #16794

docs/dev/lsp-extensions.md

nemethf · 2024-03-05T21:47:01Z

docs/dev/lsp-extensions.md

+**Request:** `RunTestParams`
+
+```typescript
+interface DiscoverTestParams {


Is this a copy'n'paste error? DiscoverTestParams is already defined in line 395. This is probably RunTestParams. I also assume the experimental/runTest request is sent from the client to the server. But what how should the client specify include and exclude. I'm guessing these are lists of TestItem ids, right? Is it okay to specify both lists at the same time?

Fixed in #16794

nemethf · 2024-03-05T21:57:03Z

docs/dev/lsp-extensions.md

+**Notification:** `ChangeTestStateParams`
+
+```typescript
+type TestState = { tag: "failed"; message: string }


Is message a plain string or markdown formatted? What happens when the client supports colorDiagnosticOutput?

It is the stderr of the test, and it usually has no color unless the program explicitly prints some ansi colored text, which in this case I think it is reported as is.

I think it would be helpful to extend the documentation explaining that the message is the standard error of the test.

nemethf · 2024-03-05T22:03:33Z

docs/dev/lsp-extensions.md

+    // request for simple execution, but for more complex execution forms
+    // like debugging, this field is useful.
+    runnable?: Runnable | undefined;
+};


for completeness (and when the client desynchronizes) would it make sense to include a state: TestState field as well? With possible additional states like: scheduled (scheduled to run) and initial.

Does the client desynchronization happens frequently? The server does not store the test state currently, and it only report it to the client. In the vscode I think it stores the previous results independent of the server, does it solve this problem?

Desynchronization is rare, and it is usually a result of a bug. But these bugs are hard to track down, so any additional info helps to find them. At any rate, the server does seem to store something about the tests otherwise how it can detect changes in existing tests and send the experimental/discoveredTests notification.

Maybe TestItem should not contain its state if that's to hard to do, but a queue-position would be definitely useful showing how many tests are before it if rust-analyzer scheduled the test to run.

I think there is value in keeping the discovery phase and running phase separate. They are separate in the vscode api, and in the r-a case, in the discovery phase the server uses its knowledge but in the running phase it just act as a proxy between cargo and the client. So I think not adding test state and queue position in the discovery related requests is good. But maybe maybe we can add more information to the run related requests for the desynchronization problem. But maybe if it is a rare and bug related event, then the user can just tolerate it rerun the tests?

Probably. Thanks for this and other clarifications and updates of the documentation.

ShuiRuTian · 2024-03-06T09:32:19Z

I did not image this PR would be merged so quickly...

I am still curious what does it looks like for now, anyway, I could try next publishment.

Veykril · 2024-03-06T09:35:55Z

I did not image this PR would be merged so quickly...

Me neither. I am not too happy with the hack to get around a cargo limitation either in this honestly, r-a already has way too much tech debt as is and this is just adding on to it.

Veykril

I haven't looked at the actual logic here yet, unsure when I'll get to that since I'll need to brush up on what was said in #3601 designwise to compare.

Veykril · 2024-03-06T09:44:43Z

crates/rust-analyzer/src/hack_recover_crate_name.rs

@@ -0,0 +1,25 @@
+//! Currently cargo does not emit crate name in the `cargo test --format=json`, which needs to be changed. This


Then we should get that fixed in cargo instead of creating this mess here

I will try to fix it in cargo, By the way @ShuiRuTian did you had better solution to this in your PR?

Not sure what here want to do.
If it's trying to find the crate of running tests, it's not a big problem in my PR. Because we do have the entire logic test tree(creates-testkinds-modules-tests) and we only allows one test to be run meanwhile, so it's easy to get the crate of the test that user clicked by calling get parent.

Aha. This PR handles running multiple tests so it hits this problem.

It seems there is an effort in upstream (see rust-lang/testing-devex-team#1 and rust-lang/rfcs#3558) about reworking the test json output that will hopefully fix this problem.

crates/rust-analyzer/src/main_loop.rs

ShuiRuTian · 2024-03-06T10:15:55Z

editors/code/src/lsp_ext.ts

+    parent?: string | undefined;
+    textDocument?: lc.TextDocumentIdentifier | undefined;
+    range?: lc.Range | undefined;
+    runnable?: Runnable | undefined;


Ah, do we have runnable at the first commit?

The info in Runnable is rich, I believe it already tells range, textDocument, and parent. It would be kind of confused if there is no single truth of source.

It doesn't tell parent, but it tells range and textDocument. I will remove the duplicate fields of runnable here.

I ended up keeping those fields, since some of them should be kept in sync with the old runnables in order to function correctly without breaking the old runnables, but are not ideal for the test explorer usage. And it makes the implementation for clients that already implement the runnable extension simpler.

ShuiRuTian · 2024-03-06T10:18:13Z

editors/code/src/lsp_ext.ts

+    runnable?: Runnable | undefined;
+};
+export type DiscoverTestResults = { tests: TestItem[]; scope: string[] };
+export type TestState = { tag: "failed"; message: string } | { tag: "passed" } | { tag: "started" };


At lease there is also cancel and ignore state. BSP is a good exmple.

Fixed in #16794. Neither cargo nor the vscode api don't have anything equivalent to cancel I think, so I omitted it for now.

ShuiRuTian · 2024-03-06T10:21:41Z

editors/code/src/ctx.ts

@@ -103,6 +105,10 @@ export class Ctx implements RustAnalyzerExtensionApi {
    ) {
        extCtx.subscriptions.push(this);
        this.statusBar = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Left);
+        this.testController = vscode.tests.createTestController(


Maybe add an option to control whether the feature is enabled? The default might be true(although it's kind of aggressive for me, because the API are all experimental), but an option is a good escape hatch if there is something wrong.

Agreed, config (defaulted to false) and a capability is the least we should do here. #16773

Why should the default be false? Does the presence of this feature interfere with normal operation?

This is hot of the press, it might have issues we don't know about and we don't know the perf impact yet (this is the important bit imo, especially if someone doesn't make use of it, we shouldn't be wasting cpu cycles on the feature). Turning it off by default (and then stating that in the changelog how to enable) seems like the better starting experience, I reckon (at least judging from the issue) that enough people are interested in this and enable to it to give us relevant testing feedback.

Veykril · 2024-03-06T18:45:09Z

@lnicola when you do the changelog for this, would be nice if you could mention that the config introduced by #16773 has this feature disabled by default, that is it is opt-in

Some minor changes in the test explorer lsp extension followup #16662 cc `@nemethf` `@ShuiRuTian`

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 24, 2024

HKalbasi force-pushed the test-explorer branch 2 times, most recently from b86d6b3 to 348847e Compare February 24, 2024 21:08

HKalbasi force-pushed the test-explorer branch from 348847e to 69bf993 Compare February 24, 2024 21:25

HKalbasi force-pushed the test-explorer branch 2 times, most recently from 2dcd1c3 to 5358443 Compare February 26, 2024 21:39

HKalbasi force-pushed the test-explorer branch 2 times, most recently from a594e1a to aa55695 Compare March 1, 2024 10:10

Add test explorer

44be243

HKalbasi force-pushed the test-explorer branch from aa55695 to 44be243 Compare March 5, 2024 20:35

HKalbasi mentioned this pull request Mar 5, 2024

Announcements: Client Changes #4604

Open

bors merged commit 767d5d3 into rust-lang:master Mar 5, 2024
11 checks passed

This was referenced Mar 5, 2024

config: Add Configuration for Progress Reporting Verbosity #16700

Closed

feat: enable automatically fix diagnostics #16715

Closed

nemethf reviewed Mar 5, 2024

View reviewed changes

Veykril reviewed Mar 6, 2024

View reviewed changes

ShuiRuTian reviewed Mar 6, 2024

View reviewed changes

Veykril changed the title ~~Add test explorer~~ feat: Add test explorer Mar 6, 2024

HKalbasi mentioned this pull request Mar 8, 2024

Some minor changes in the test explorer lsp extension #16794

Merged

bors added a commit that referenced this pull request Mar 9, 2024

Auto merge of #16794 - HKalbasi:test-explorer, r=lnicola

574e23e

Some minor changes in the test explorer lsp extension followup #16662 cc `@nemethf` `@ShuiRuTian`

philips-software-forest-releaser bot mentioned this pull request Jun 4, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#436

Merged

This was referenced Jun 11, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#445

Merged

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#447

Merged

philips-software-forest-releaser bot mentioned this pull request Jun 18, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#456

Merged

philips-software-forest-releaser bot mentioned this pull request Jun 25, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#469

Merged

		@@ -0,0 +1,25 @@
		//! Currently cargo does not emit crate name in the `cargo test --format=json`, which needs to be changed. This

feat: Add test explorer #16662

feat: Add test explorer #16662

Conversation

HKalbasi commented Feb 24, 2024

HKalbasi commented Feb 24, 2024

ShuiRuTian commented Feb 26, 2024

HKalbasi commented Feb 26, 2024

ShuiRuTian commented Feb 27, 2024

HKalbasi commented Mar 1, 2024

bors commented Mar 2, 2024

HKalbasi commented Mar 5, 2024

bors commented Mar 5, 2024

bors commented Mar 5, 2024

bors commented Mar 5, 2024

nemethf 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

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

ShuiRuTian commented Mar 6, 2024

Veykril commented Mar 6, 2024 • edited Loading

Veykril 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

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

Veykril Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Veykril commented Mar 6, 2024

Veykril commented Mar 6, 2024 •

edited

Loading

Veykril Mar 6, 2024 •

edited

Loading