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

[core] Migrate test runner to vitest #71

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

brijeshb42
Copy link
Contributor

Note that the class names for the generated css have changed because earlier, the root directory was considered from repo root, but now, it considers the package directory as the root. And classname generation depends on this path.

@brijeshb42 brijeshb42 added the core Infrastructure work going on behind the scenes label May 10, 2024
@oliviertassinari oliviertassinari added the scope: code-infra Specific to the core-infra product label May 14, 2024
@oliviertassinari
Copy link
Member

What's the motivation for this change?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 29, 2024
@zannager zannager requested a review from mnajdova June 3, 2024 15:06
@Janpot
Copy link
Member

Janpot commented Jun 5, 2024

For historic reasons Toolpad ended up using vitest instead of mocha for its unit tests. Current Toolpad could benefit from some of the test infra of core and maybe we'll try again to switch, but I have to admit, it's a lot less painful compared to karma+babel+mocha. Can't explain the the motivation for this change, but I can share my experience with vitest:

pros:

  • faster and parallel
  • no need for babel, runs typescript/jsx code out-of-the-box
  • actually handles module resolution correctly as expected
  • built-in mocking (looking at you Datepickers for testing multiple versions of date-fns)
  • no need for karma to run tests in the browser
  • it just works

cons:

  • can't reuse mui test utils
  • no babel means potentially testing differently compiled code.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 5, 2024

How do we run the tests in a real browser with vitest? As I understand things:

  • The reason we use mocha is so we can run tests in a browser with Karma.
  • We can't use playwright because it's x10 slower than Karma, and Karma already needs to be x2 faster to have a good PR DX.
  • We can't use jsdom only because it's not accurate enough, at least anytime we go out of the standard React rendering flows, e.g. data grid, I believe @romgrk experience was too limited.

Now https://github.com/emotion-js/emotion/, https://github.com/callstack/linaria/, https://github.com/styled-components/styled-components/, seem to run tests with jest (and maybe jsdom) only.

https://vitest.dev/guide/browser.html + https://vitest.dev/guide/improving-performance.html, oh maybe could solve two pain points 🤔

cons: no babel means potentially testing differently compiled code.

We should transpile the source to be close to production, but I guess this won't be an issue.

const: can't reuse mui test utils

We should be able to reproduce equivalent utils.

@Janpot
Copy link
Member

Janpot commented Jun 5, 2024

How do we run the tests in a real browser with vitest?

Yep, they're adding a browser mode. Granted, it's not declared stable yet, but I made it work today in Toolpad in relatively short time. It was that or move to mocha+karma. I gave it a quick shot, and it's quite delightful, still not supporting the full functionality of the node.js runner, but certainly seems to be usable for us.

We should transpile the source to be close to production, but I guess this won't be an issue.

vitest transpiles the code for you, with vite. It's a trade-off I'm personally willing to make. I think it could make sense to try and cover this use-case with a few integration tests instead. Fewer tests, but closer to actual production code.

@JCQuintas
Copy link
Member

We should transpile the source to be close to production, but I guess this won't be an issue.

Would moving to vite instead of babel be a huge issue? We have some complex babel transpiling in X that I'm not sure would be portable to vite.

@Janpot
Copy link
Member

Janpot commented Jul 24, 2024

We have some complex babel transpiling in X that I'm not sure would be portable to vite.

Which transformations do you think aren't portable? The one I'm aware of is the macro in MUI core for error messages. I don't think X is even making use of that? In any case, I don't think there is anything preventing us from using a babel transform in vite if necessary. But I'd love to get rid of the macro system altogether if we can, making our code portable to other compilers would be a big win.

IMO transforming the test format from mocha+chai to vitest will probably be the most intensive part of the process.

@JCQuintas
Copy link
Member

Oh, I was mostly asking if there are known issues, I don't have a lot of exp with vite/vitest. But I would guess on X the date adapters import replacement could be an issue.

@Janpot
Copy link
Member

Janpot commented Jul 24, 2024

But I would guess on X the date adapters import replacement could be an issue.

😄 Actually mentioning this exact use-case in my comment above.

  • built-in mocking (looking at you Datepickers for testing multiple versions of date-fns)

I expect that we can build that on top of vitest module mocking. I'm not a huge fan of this method as it's quite a lot of magic, but it should be much more straightforward to do from DX point of view. 🤷 As long as we're not starting to overuse this feature everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes PR: out-of-date The pull request has merge conflicts and can't be merged scope: code-infra Specific to the core-infra product test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants