-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Related tests #7799
Related tests #7799
Conversation
crates/rust-analyzer/src/lsp_ext.rs
Outdated
pub enum RelatedTests {} | ||
|
||
impl Request for RelatedTests { | ||
type Params = RelatedTestsParams; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type Params = RelatedTestsParams; | |
type Params = lsp_types::TextDocumentPositionParams; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to expand the RelatedTestsParams in the future.
For example to support microsoft/vscode#107467. I saw the #5765 but seems it's abandoned. And in my opinion VSCode native tests support is preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps RelatedTests
is not the best name for the request, but just Tests
is even worse, in my opinion. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to expand the RelatedTestsParams in the future.
We'll add RelatedTestsParams
with serde flatten at that point
Perhaps RelatedTests is not the best name for the request,
it sounds ok to me tbh. We can always change later.
And yeah, it'd be lovely to switch to upstream impl for running, debugging and testing, but we realistically can do that only when upstream supports "run thing at point" workflow. Not sure what's the status there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to expand the RelatedTestsParams in the future.
For example to support microsoft/vscode#107467. I saw the #5765 but seems it's abandoned. And in my opinion VSCode native tests support is preferable.
It has been requested, but for now, I am finishing my diploma and then I will return to the project. These are not only tests. It's Runnable. That is, tests, examples, entry functions. If you want, you can implement an approximate date when I plan to return to it at the beginning of the next month. Yes, for only test i planed use native explorer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to expand the RelatedTestsParams in the future.
For example to support microsoft/vscode#107467. I saw the #5765 but seems it's abandoned. And in my opinion VSCode native tests support is preferable.
Hey @vsrs - thanks for this awesome feature!
I am implementing the emacs lisp lsp-mode client code for this extension (my WIP PR here emacs-lsp/lsp-mode#2776) and i wanted to clarify why you decided to return TestInfo[]
as a wrapper over Runnable
instead of returning Runnable[]
? Using the same interface as already implemented runnable methods makes it easier to integrate now and enables future code re-use.
Do you intent to expand change the protocol and add more keys to TestInfo? If not, would you and @matklad be ok if I open a PR that changed the Request surface for RelatedTests (and all the corresponding front-end callsites?)
impl Request for RelatedTests {
type Params = lsp_types::TextDocumentPositionParams;
type Result = Vec<Runnable>;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you intent to expand change the protocol and add more keys to TestInfo?
Yes, I do. I'm working on a Rust test provider\adapter using upcoming VSCode API ( microsoft/vscode#107467) and I reuse expanded TestInfo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use vscode but i would like to bring as much OOTB goodness from vscode as possible, so would appreciate your input.
The link you provided is very comprehensive and since I cannot ctrl-f TestInfo
anywhere in the document, can you please provide some detail. where and how is testinfo going to be used?
just for me to understand - will you be modifying this rust-analyzer endpoint or do you intend to add new ones? If the former, will you be breaking backwards compat of the interface and what fields do you want to add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current VSCode test adapter API design is host only and hasn't LSP part, so TestInfo
is my own abstraction and it's only a draft. I cannot say right now how it will be changed in the future. But as every test is a runnable, TestInfo
will always have the pub runnable: Runnable,
field. I expect that rust-analyzer/relatedTests
request will not use new fields\structures, that is why I decided to publish this feature before the test adapter itself.
@@ -106,6 +110,92 @@ pub(crate) fn runnables(db: &RootDatabase, file_id: FileId) -> Vec<Runnable> { | |||
res | |||
} | |||
|
|||
pub(crate) fn related_tests( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a // Feature: Related Tests
comment, otherwise no one will know that the thing exists )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, right!
bors r+ |
This adds an ability to look for tests for the item under the cursor: function, constant, data type, etc
The LSP part is bound to change. But the feature itself already works and I'm looking for a feedback :)