Vitest 🚀 #4628
Vitest 🚀 #4628
Conversation
apparently, we're using a few things that are not supported in happy-dom
for coverage inside vs code
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
.github/workflows/ci.yml
Outdated
| - name: Run tsc build | ||
| run: yarn buildTsc | ||
|
|
||
| - name: Run tsc buildTests | ||
| run: yarn lerna run buildTests |
There was a problem hiding this comment.
Added side effect: The istJustJavascript and dist/tests dirs are pretty much things of the past.
I haven't entirely gone rid of them at this point, but we're pretty close now!
There was a problem hiding this comment.
When I last looked into this, i.e. when I introduced tsx, the main reason to keep istJustJavascript was that tsx runs esbuild every time, so we pay the cost of the transpilation on every restart of the process, e.g. the admin server, and also I wasn't sure how much ongoing overhead it incurs during runtime.
I'm now more inclined to think it would be fine to use it in prod, but it warrants more investigation. See also their FAQ.
There was a problem hiding this comment.
Yes - this is certainly not something I want to change in this PR :)
Was just thinking that jest was one of the main consumers of itsJustJavascript, and that's now gone.
adminSiteServer/app.test.ts
Outdated
There was a problem hiding this comment.
Mostly whitespace changes in here (review with ?w=1), and then also some changes to how the mocking of googleapis works.
db/Variable.test.ts
Outdated
There was a problem hiding this comment.
This test suite I had to disable (using it.skip), because the mocking doesn't work: see PR description.
| // https://github.com/d3/d3-geo-projection/issues/202 | ||
| // todo: would be nice to get propert types for this and not have to run different code during testing | ||
| const projectionToUseDuringJestTesting = geoConicConformal() |
There was a problem hiding this comment.
Could just get rid of this without any issues, so 🤷🏻
|
Quick links (staging server):
Login:
SVG tester:Number of differences (default views): 0 ✅ Edited: 2025-03-05 12:33:59 UTC |
There was a problem hiding this comment.
For whatever reason, we had both a MarimekkoChart.jsdom.test.tsx file and a MarimekkoChart.test.tsx - and both of them neither required jsdom nor were they TSX 😬
So, I merged them into one single file. Nothing changed otherwise.
| @@ -7,17 +11,16 @@ import { | |||
| slugify, | |||
| } from "@ourworldindata/utils" | |||
| import { fireEvent, render, screen } from "@testing-library/react" | |||
| import "@testing-library/jest-dom" | |||
| import "@testing-library/jest-dom/vitest" | |||
There was a problem hiding this comment.
Despite jest being in the name, this really is the way to setup testing-library for vitest.
There was a problem hiding this comment.
Very cool. The future is bright!
Do you have a plan for what to do about the disabled test suites? I'm worried we might forget about them for a long time.
Could we schedule to fix them, e.g. for the next cooldown? Or should we consider deleting them instead, if they are not very valuable (I don't know whether they are).
.github/workflows/ci.yml
Outdated
| - name: Run tsc build | ||
| run: yarn buildTsc | ||
|
|
||
| - name: Run tsc buildTests | ||
| run: yarn lerna run buildTests |
There was a problem hiding this comment.
When I last looked into this, i.e. when I introduced tsx, the main reason to keep istJustJavascript was that tsx runs esbuild every time, so we pay the cost of the transpilation on every restart of the process, e.g. the admin server, and also I wasn't sure how much ongoing overhead it incurs during runtime.
I'm now more inclined to think it would be fine to use it in prod, but it warrants more investigation. See also their FAQ.
.vscode/launch.json
Outdated
There was a problem hiding this comment.
Having a Vitest configuration shared across the team would be nice. Could we add one?
There was a problem hiding this comment.
Yeah, sure, although I must say that I don't really use Launch configurations myself.
I'm also gonna share the vitest VS Code extension, which is really quite cool and useful!
But, feel free to add a launch config yourself, if you feel like it.
|
Report too large to display inline |
I actually could re-enable two of them, it was relatively easy to do for them. The only thing that's still non-trivial is |
|
Thanks for your fast review, btw, really appreciate it! |
This makes our tests run way smoother & more awesome by switching from Jest to Vitest!
This means we don't need any compilation step any more, and can watch test files with incredible speed. This is super nice!
Review notes:
import { it, expect, describe } from "vitest"to the test.jsdom.test.tsx?test files to be named.test.tsx?, too - and instead added a@vitest-environment jsdomcomment at the top. It also turned out that many of them didn't even need the JSDom env at all - for those I've removed it.a,bare both defined inmodule.ts, andacallsb, then you can't mockbfrom the outside (or, well, it won't have any effect).