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

Feature request: Make "--changed" respect coverage or use static code analysis #6735

Open
4 tasks done
ffMathy opened this issue Oct 17, 2024 · 5 comments
Open
4 tasks done

Comments

@ffMathy
Copy link

ffMathy commented Oct 17, 2024

Clear and concise description of the problem

What if Vitest kept track of which lines each test runs through. That way, the "--changed" flag would be truly only "changed", and could save a huge amount of test execution time.

In our test suite, we have a lot of end-to-end tests, as well as a lot of integration tests. So here, we don't care about Vitest's performance, because that's typically not the limiting factor. IO such as database operations and API calls is limiting instead.

So for that, to really cut off some test execution time, we need to be able to better restrict what tests are run.

--affected does a great job already by looking at the import tree, but it could become even better. If this was implemented, it would be equal to how it works in IntelliJ based IDEs like WebStorm, IntelliJ and Rider, and it could be one of the only test libraries for node that would support this. So there's a lot of opportunity here.

Suggested solution

The way I see it, it could be achieved in three ways.

  1. Using code coverage: Some tools already do this today. However, it may require generating coverage in a specific format which includes this information. Perhaps there's also some performance considerations in basing it on files (so maybe suggestion 2 is better).
  2. Using in-memory code coverage: Same as above, but maybe based on some kind of in-memory data. Vitest must generate existing coverage files today based on some data in memory (that's an assumption), so this could maybe be "easy" to implement directly into the "watch" mechanism of Vite.
  3. Analyzing the AST: Similarly to how imports are analyzed today, we could use the Visitor design pattern or something similar to be able to recursively traverse function calls etc. But that might be more complex, as Node projects can be configured in a variety of ways.

Alternative

No response

Additional context

This was sparked here: #6734

Validations

@sheremet-va
Copy link
Member

sheremet-va commented Oct 17, 2024

Sorry, but there is no --affected flag. Do you mean --changed? Or do you want a separate flag?

@ffMathy
Copy link
Author

ffMathy commented Oct 17, 2024

Ah yes, brain fart from my side. I did mean changing the existing flag's behavior.

@ffMathy ffMathy changed the title Feature request: Make "--affected" respect coverage or use static code analysis Feature request: Make "--changed" respect coverage or use static code analysis Oct 17, 2024
@ffMathy
Copy link
Author

ffMathy commented Oct 23, 2024

Will you accept PRs for this?

@sheremet-va
Copy link
Member

All suggested solutions seem like overkill to me. The second one seems impossible because Vitest doesn't have an in-memory coverage as far as I know (I might be wrong). To analyse AST, we need to process files with Vite into JavaScript first, which is not a problem and might be the easiest solution out of all of them, to be honest. (Btw, how do you parse dynamic test names?) To even work with coverage, you need to generate it, which means you need to store it just for --changed, which seems very inefficient to me and will just pollute the git repo (assuming you are pushing it to use in CI).

This seems like a huge project, and I don't think anyone on the team wants to maintain it (We usually say no to features we can't maintain because Vitest is already pretty big). This will also require a big refactoring of the internals to have a map of tests to files ahead of time.

Maybe others on the team have a different opinion? @hi-ogawa @AriPerkkio @antfu

@AriPerkkio
Copy link
Member

As said before, in order to use coverage for this we would first need to run all tests so that we have coverage results. That kind of breaks the intention of --change completely. And coverage is collected after a single test file has run. If we wanted to know what files/lines a single test executed, we would need to change that logic too. I'm not sure what test.concurrent would look like.

Are there any existing Javascript tools that have implemented this? What kind of logic would the static analysis part have? Implementing static analyses for dynamic cases is simply not possible. We need to evaluate the code for many cases. Tests are often very dynamic.

Will you accept PRs for this?

I'm quite curious how you would even start implementing this. 😄
Feel free to build some proof-of-concept but it's likely that this won't be accepted.

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

No branches or pull requests

3 participants