rust-analyzer
rust-analyzer copied to clipboard
rust-lang
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
@ShuiRuTian Can you please review this? You can probably detect its problems since you have implemented this functionality one time :)
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 https://github.com/rust-lang/rust-analyzer/issues/3601#issuecomment-945778285. 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.
@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 https://github.com/rust-lang/rust-analyzer/issues/3601#issuecomment-945778285 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!
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.
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.
:umbrella: The latest upstream changes (presumably #16704) made this pull request unmergeable. Please resolve the merge conflicts.
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+
:pushpin: Commit 44be2432f50be8d7476f65b18220027110d7bd99 has been approved by HKalbasi
It is now in the queue for this repository.
:hourglass: Testing commit 44be2432f50be8d7476f65b18220027110d7bd99 with merge 767d5d3eab237a1fbc10d62b120020b12f6026c5...
:sunny: Test successful - checks-actions Approved by: HKalbasi Pushing 767d5d3eab237a1fbc10d62b120020b12f6026c5 to master...
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.
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.
@lnicola when you do the changelog for this, would be nice if you could mention that the config introduced by https://github.com/rust-lang/rust-analyzer/pull/16773 has this feature disabled by default, that is it is opt-in