Skip to content
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

Closed

Conversation

godcodehunter
Copy link
Contributor

@godcodehunter godcodehunter commented Aug 15, 2020

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:

  • the programmer should be able to complete tests using cargo test
  • debug by any debuggers convenient to him
  • if the test somehow interacts with stdin to print there

My counterarguments:

  • VS Code already have at Test API ability for kill test, get raw output
  • As I saw VS Code also have the ability for the persistent test but it's limited
  • We should be fronted agnostic if vs code will be suspended or we want to use another front/tools, we lost information so in this case, we should keep vs code open if we want to get info about runs
  • VS Code API for providing runed related to workspace instance or we can provide PID and another info bu API and some way show it for user
  • In the future, we need such information for implementation to various possibilities features, such as:

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.

@matklad
Copy link
Member

matklad commented Aug 18, 2020

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.

@kjeremy
Copy link
Contributor

kjeremy commented Aug 18, 2020

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.

@godcodehunter
Copy link
Contributor Author

godcodehunter commented Aug 19, 2020

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.

@vsrs vsrs mentioned this pull request Mar 3, 2021
@Milo123459
Copy link
Contributor

Any updates on this PR?

@godcodehunter
Copy link
Contributor Author

godcodehunter commented Jul 25, 2021

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.
Another important point is that the test api has been standardized recently. And as I understand it partially, there are also points in the roadmap for July. So there is time until the end of the month)

@dbofmmbt
Copy link
Contributor

That will be a great addition. I'm looking forward to test it once you're done implementing it.

@godcodehunter
Copy link
Contributor Author

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.

@matklad
Copy link
Member

matklad commented Oct 18, 2021

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:

  • adding IDE feature shouldn't require changing db setup
  • we shouldn't try to expose "get live view of every X in a project" functionality, as it is non-scalable
  • test process spawning should be the responsibility of the client

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support VS Code testing API
5 participants