-
Notifications
You must be signed in to change notification settings - Fork 1.7k
WIP: test and runnable explorer #5765
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
WIP: test and runnable explorer #5765
Conversation
cc @vsrs Note that we are moving (slowly) the TypeScript to github.com/rust-lang/vscode-rust. I am not sure how this would pan out, but there's a chance that this PR would have to be reapplied against that other repo. |
Does this integrate with the existing Rust Test Explorer extension? I think an alternate approach would be to hook into https://marketplace.visualstudio.com/items?itemName=hbenl.vscode-test-explorer by writing an adapter. |
No it will use its own implementation. There are a couple of features that I would like to implement. For example rust have bench test, i would like show pass time inside row in right side and etc. |
d3c3d74
to
bc723c8
Compare
Any updates on this PR? |
Delta update implementation. But most likely I will remove them. And I will separate it into a separate pr. Now it remains to add some new api and permanently updated runnable tree (you can see that tree in TS code). So I think there is not much time left to wait, but a little more is needed. |
bc723c8
to
cf594cc
Compare
That will be a great addition. I'm looking forward to test it once you're done implementing it. |
Yes, I myself want to quickly implement this. To be honest, I'm even a little ashamed that it took so long. There is not much left, now I am implementing a salsa database. I plan to finish the first usable stage by simply running tests. But there are a couple more tasks that I will most likely put into separate pull requests. |
So this is clearly a lot of work, but I don't think this moves in the right direction. The diff here is big, it changes rust-analyzer in a rather fundamental way (adds a new db trait internally, adds a new incremental diffing on the LSP layer), and it isn't actually finished. I don't think this moves in the right direction, on many levels:
I don't have time to argue each and every specific point in detail, the main thing being is that it's a too large a change. I am going to close this PR to unblock alternative approaches to implementing the feature. |
Resolves: #3601
The arisen issues:
Сonversation summary on 6/8/2021: matklad and godcodehunter
1. The complexity of the test detection algorithm for the entire project remains incremental. But the algorithm still remains offline.
This means that we will get the result for the entire project when it is ready, but not updates as the algorithm progresses.
A small example to explain:
Imagine that we want to calculate the sum of all array values.
And our sum is split into some chunks of 12 elements. Well, I don’t know, let it be files on a disk with such a strange format. When we want to calculate the amount, some individual chunks may already be calculated and cached. But in general, we will return the result to the user only when the chunks are counted. Instead, we could give the user a list of chunks that are out of date and gradually serve updated chunks sums.
It is that what I mean when say algorithm incremental but still offline.
In fact, I already knew about this problem at the time of the conversation. And the ability to implement offline is built into the API.
Then what's the problem?
The problem is first on the VS Code side.
Secondly, this is the additional time that will need to be spent on implementing tree synchronization. And I was planning to do it in the next PR anyway.
2. Responsibility for running tests
We had a dispute about who really owns the tests and how they run.
Kladov put forward the following idea that the editor should be responsible for launching, since:
cargo test
My counterarguments:
3. Update optimization problem
Kladov notes that now, even taking into account the incrementality, such an approach can be expensive in terms of computation. The reason is in the design of the check algorithm and macro expansion.
I suggest several improvements to the current model below:
First of all, I think the first place to start is of course add profiling for this.
Observing traversal
Currently, when we need to do any analysis, we just take the cached AST from salsa. And traverse it step by step by doing something check. It is quite obvious, that makes no sense to bypass whole AST every time if only some of its parts have changed.
Now I will give an example of how it might look apart from the RA. And in the end I will summarize what is needed for this in RA.
TODO
(Summary writing in progress, I will finish today)
All of the above, with the exception of the responsibility issue, are non-blocking issues and would resolve in the next PRs.