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

Replace Jest with Vitest 2 #4728

Merged
merged 32 commits into from
Nov 22, 2024
Merged

Replace Jest with Vitest 2 #4728

merged 32 commits into from
Nov 22, 2024

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Sep 20, 2024

This PR replaces the test runner (unit/integration/build) from Jest to Vitest 2, which has built-in support for TS/ESM and the same syntax as Jest. This PR serves to help us evaluate the option, and to move forward faster if there's a decision to proceed at some point.

A key motivation for examining this change is that I've found it quite challenging to move these tickets forward:

Part of the struggle is that Jest only has experimental ESM support, and I've so far not managed to see an effect of the useESM setting in the ts-jest plugin used in this repo.

It's a continuation of

Relates to Rolldown, because vitest is downstream of vite, and thus eventually will swap internals to use rolldown:

Relates a bit to browser runner, because vitest is working on a browser mode:

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!

@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.80%. Comparing base (97c909f) to head (e4b0e48).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4728      +/-   ##
==========================================
+ Coverage   89.11%   91.80%   +2.68%     
==========================================
  Files         269      279      +10     
  Lines       38392    38332      -60     
  Branches     2379     6698    +4319     
==========================================
+ Hits        34214    35190     +976     
+ Misses       3160     3008     -152     
+ Partials     1018      134     -884     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@birkskyum birkskyum force-pushed the vitest-2.0 branch 2 times, most recently from 4762bb5 to 9386082 Compare September 20, 2024 14:12
@birkskyum birkskyum changed the title Replace Jest to Vitest 2 Replace Jest with Vitest 2 Sep 21, 2024
@birkskyum birkskyum marked this pull request as ready for review September 21, 2024 21:24
@birkskyum birkskyum force-pushed the vitest-2.0 branch 4 times, most recently from 47ea443 to 28ec0ae Compare October 31, 2024 16:14
@birkskyum birkskyum mentioned this pull request Nov 7, 2024
@jasongitmail
Copy link

@birkskyum I took a look at the unit test config as you requested.

Looks good afaict. Coverage is in a separate command, fileParallelism is left at the default (true), etc.

If there is a particular file that consistently runs more slowly, using describe.concurrent() within it could help. And I'd carefully look at how beforeEach() is used and consider if beforeAll() would be sufficient.

I don't claim to be a Vitest expert though. I switched from Jest -> Vitest -> Bun, and experienced speed wins each time. But you're using newer versions of Jest and Vitest, so things may have changed. And these tests use JSDom, which I've never needed to use, which is slower than running tests on node.

The benefit of this PR, is not only does Vitest provide ESM support, it brings some future proofing because Vitest should eventually be rewritten in native code, given VoidZero's vision for a rust based end-to-end toolchain.

@birkskyum
Copy link
Member Author

birkskyum commented Nov 16, 2024

@jasongitmail , thank you very much for the review. It is a good idea to try to narrow down single files that might be impacting the numbers, and if beforeEach/All .concurrent() can help. I haven't looked into either of those, I just ported the tests directly, so it's likely there's something to gain.

You can run Vitest with Bun, and see a speedup? I haven't had much success with that. Unless the --bun flag is set, I believe bun just fires up node to run vitest. If you can run with this flag, please let me know, because then I have to update some tickets. If you can see a speedup running without the --bun flags, that's quite surprising to me given it's likely the same node process as not using bun, but they might of course have a trick.

@jasongitmail
Copy link

I didn't run Vitest with Bun, I ported my tests to bun:test by changing the imports import { expect, test } from "bun:test";. In my case, it mostly just worked because bun:test aims to be Jest compatible. https://bun.sh/docs/cli/test

It's an option, but I'm not suggesting that in this case b/c maplibre-gl-js probably has many contributors and this would require them to have Bun installed to run tests, and I'd guess you'll likely end up switching back to Vitest down the road for ecosystem alignment. But it could be fun to play with to see how fast tests can be.

@birkskyum
Copy link
Member Author

birkskyum commented Nov 19, 2024

@jasongitmail aah, that makes sense - the stats I've seen for bun:test are quite impressive, but I have yet to try it, because it wasn't an option for this PR. As you say, it's requiring everyone to move to another runtime which would take a very convincing argument, and also until the day bun get a text-based lock-file, we'd be unable to use Dependabot, which we rely heavily on.

@birkskyum
Copy link
Member Author

This describes some of the differences between the two runners

https://cookbook.marmicode.io/angular/why-vitest/

@HarelM
Copy link
Collaborator

HarelM commented Nov 21, 2024

My main concern is about coverage, I've spent a great deal of time to make the coverage work correctly for both render and unit tests.
I still think it doesn't work well with latest jest (there's an open issue on it), but at least it's close to working correctly...

@birkskyum
Copy link
Member Author

birkskyum commented Nov 21, 2024

@HarelM , do you mean you're generally concerned with coverage (that it's important), or that you have specific concerns that vitest is worse when it comes to coverage?

Is below how you'd expect it to look for the unit tests? it's the npm run test-unit-ci

Vitest is more flexible than jest when it comes to coverage. There are 3 different coverage providers ('v8' | 'istanbul' | 'custom' ) with default being the v8

Screenshot 2024-11-21 at 23 24 52

@birkskyum
Copy link
Member Author

This is the npm run test-integration-ci

Screenshot 2024-11-21 at 23 26 09

@HarelM
Copy link
Collaborator

HarelM commented Nov 22, 2024

I'd like to solve the current problem we have we coverage, this might be an opportunity to do so while removing the need to use all kind of crazy package we have today like monocart etc.
Can you configure the CI to upload the coverage reports (json files) to codecov so that I can take a look at things I know are not reported correctly today?

@birkskyum
Copy link
Member Author

birkskyum commented Nov 22, 2024

It's passing the unit+integration .json from vitest to codecov now, and the normal test-render .json.

@HarelM , can you help verify it's correctly picked up in there?

@HarelM
Copy link
Collaborator

HarelM commented Nov 22, 2024

It reports 70k lines, originally it reported arond 40k.
I'll need to see if it reports comments and non functional code...

@birkskyum
Copy link
Member Author

birkskyum commented Nov 22, 2024

@HarelM , I made a mistake just before (see commit) - it double counted the integration tests. should be resolved now

@birkskyum
Copy link
Member Author

birkskyum commented Nov 22, 2024

note: This still uses monocart for the render tests still (but no longer for integration / unit)

We might be able to use the Vitest Browser Mode (experimental) for render tests later on, but I found it was probably best to evaluate it in separate PR

@birkskyum
Copy link
Member Author

birkskyum commented Nov 22, 2024

The discrepancy in reported numbers originate from vitest including the .test.ts files in the coverage.

I'll config to exclude them.

@birkskyum
Copy link
Member Author

birkskyum commented Nov 22, 2024

@HarelM I'm seeing the expected number of tracked lines now 38k +/- 60

Copy link
Collaborator

@HarelM HarelM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I'm happy to see that the problem with the coverage is not fixed with this change.

@birkskyum birkskyum merged commit 3f1686e into maplibre:main Nov 22, 2024
17 checks passed
@birkskyum
Copy link
Member Author

birkskyum commented Nov 22, 2024

I'm happy to see that the problem with the coverage is not fixed with this change.

Is there still a problem? This codecov v5 that it's not reporting correctly in PRs, is that still ongoing after this change?

@HarelM
Copy link
Collaborator

HarelM commented Nov 23, 2024

@birkskyum I've updated the repo and ran the tests locally. It takes around 14 seconds to complete the run. It used to take around 5 seconds with jest. Any ideas how to get it back to 5 seconds?

@birkskyum
Copy link
Member Author

birkskyum commented Nov 23, 2024

I do have some idea - I outlined them here. I went from 8s to 12s for unit tests in my dev environment.

@HarelM
Copy link
Collaborator

HarelM commented Nov 23, 2024

Fair enough, I'll continue monitoring the behavior.

@HarelM HarelM mentioned this pull request Nov 24, 2024
3 tasks
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.

4 participants