[code-infra] vitest browser & jsdom modes
#14508
+3,727
−2,379
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| name: Vitest | ||
|
There was a problem hiding this comment. CircleCI please: https://www.notion.so/mui-org/code-infra-Migrate-out-of-CircleCI-42350363b7344380a9961cf9731aae31 for the final version of this effort. There was a problem hiding this comment. Yeah, this is just for ensuring the changes work while working on them There was a problem hiding this comment. Can we remove the GH workflow given that CircleCI is already setup? 🤔 |
||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - 'master' | ||
| - 'next' | ||
| pull_request: | ||
| branches: | ||
| - 'master' | ||
| - 'next' | ||
|
|
||
| permissions: {} | ||
|
|
||
| jobs: | ||
| vitest-jsdom: | ||
| name: Vitest Tests (jsdom) | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 | ||
| - uses: pnpm/action-setup@fe02b34f77f8bc703788d5817da081398fad5dd2 # v4.0.0 | ||
| with: | ||
| run_install: false | ||
| - name: Use Node.js 20.x | ||
| uses: actions/setup-node@1e60f620b9541d16bece96c5465dc8ee9832be0b # v4.0.3 | ||
| with: | ||
| node-version: 20 | ||
| cache: 'pnpm' # https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#caching-packages-dependencies | ||
| - run: pnpm install --frozen-lockfile | ||
| - name: Install Playwright Browsers | ||
| run: pnpm playwright install --with-deps | ||
| - name: Run Tests | ||
| run: pnpm vitest --project "jsdom/*" | ||
| vitest-browser: | ||
| name: Vitest Tests (browser) | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 | ||
| - uses: pnpm/action-setup@fe02b34f77f8bc703788d5817da081398fad5dd2 # v4.0.0 | ||
| with: | ||
| run_install: false | ||
| - name: Use Node.js 20.x | ||
| uses: actions/setup-node@1e60f620b9541d16bece96c5465dc8ee9832be0b # v4.0.3 | ||
| with: | ||
| node-version: 20 | ||
| cache: 'pnpm' # https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#caching-packages-dependencies | ||
| - run: pnpm install --frozen-lockfile | ||
| - name: Install Playwright Browsers | ||
| run: pnpm playwright install --with-deps | ||
| - name: Run Tests | ||
| run: pnpm vitest --project "browser/*" | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -28,17 +28,30 @@ const isJSDOM = /jsdom/.test(window.navigator.userAgent); | ||
| describe('BarChart - click event', () => { | ||
| const { render } = createRenderer(); | ||
|
|
||
| beforeEach(() => { | ||
| if (window?.document?.body?.style) { | ||
| window.document.body.style.margin = '0'; | ||
| } | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (window?.document?.body?.style) { | ||
| window.document.body.style.margin = '8px'; | ||
| } | ||
| }); | ||
|
There was a problem hiding this comment. Some charts tests are reliant screen position, and margins are different for karma and vitest, so we force one. 🙃 |
||
|
|
||
| describe('onAxisClick', () => { | ||
| it('should provide the right context as second argument', function test(t = {}) { | ||
| if (isJSDOM) { | ||
| // can't do Pointer event with JSDom https://github.com/jsdom/jsdom/issues/2527 | ||
| // @ts-expect-error to support mocha and vitest | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-expressions | ||
| this?.skip?.() || t?.skip(); | ||
|
There was a problem hiding this comment. You will see this pattern across multiple files, Without this mocha will fail because it waits for the This can then be migrated to just a .skip, but vitest offers many ways of skipping test. function test(t) {
if (isJSDOM) {
t.skip();
}
}There was a problem hiding this comment. I added a shim in The same couldn't be done for Don't necessarily have to use it, but it can keep the amount of code changes in tests down. |
||
| } | ||
| const onAxisClick = spy(); | ||
| render( | ||
| <div | ||
| style={{ | ||
| width: 400, | ||
| height: 400, | ||
| }} | ||
| @@ -81,16 +94,17 @@ describe('BarChart - click event', () => { | ||
| }); | ||
| }); | ||
|
|
||
| it('should provide the right context as second argument with layout="horizontal"', function test(t = {}) { | ||
| if (isJSDOM) { | ||
| // can't do Pointer event with JSDom https://github.com/jsdom/jsdom/issues/2527 | ||
| // @ts-expect-error to support mocha and vitest | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-expressions | ||
| this?.skip?.() || t?.skip(); | ||
| } | ||
| const onAxisClick = spy(); | ||
| render( | ||
| <div | ||
| style={{ | ||
| width: 400, | ||
| height: 400, | ||
| }} | ||
| @@ -155,16 +169,17 @@ describe('BarChart - click event', () => { | ||
| ).to.deep.equal(['pointer', 'pointer', 'pointer', 'pointer']); | ||
| }); | ||
|
|
||
| it('should provide the right context as second argument', function test(t = {}) { | ||
| if (isJSDOM) { | ||
| // can't do Pointer event with JSDom https://github.com/jsdom/jsdom/issues/2527 | ||
| // @ts-expect-error to support mocha and vitest | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-expressions | ||
| this?.skip?.() || t?.skip(); | ||
| } | ||
| const onItemClick = spy(); | ||
| render( | ||
| <div | ||
| style={{ | ||
| width: 400, | ||
| height: 400, | ||
| }} | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this regex, it seems hard to search in the codebase for, e.g.
.tsx, I suspect that listing all the permutations would be clearer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why you would need to search for that though. As you will have to sweep over all configuration files when doing a change that would require updating that anyways 😅
Replacing it to list all the permutations is quite unnecessary in my view:
{.ts,.tsx,.js,.cjs,.cjsx,.mjs,.mjsx,.cts,.ctsx,.mts,.mtsx}We could cleanup some unlikely exts like
{.ts,.tsx,.js,.cjs,.mjs,.mts}which would be simpler, but less complete.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it actually be great? I assume we will never need those other extensions, so if someone create them, he might be more likely to realize those are wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite the contrary, we sometimes need those extensions:
tsx), we need a way to signal node which module system is being used.next/documentin ESM.esbuild,tsxandvitestrely on the extension to determine whether JSX needs to be transformed or not. This is currently giving problems as we have a bunch of jsx in.jsfiles. See Enable JSX in.jsfiles privatenumber/tsx#644 and Allow JSX in .js files vitest-dev/vitest#1564. For the latter I have a workaround but it also forces our.tsfiles to adhere to the JSX rules, which required me to refactor some code.We shouldn't restrict extensions, they're not for cosmetic reasons. We should just prefer
.ts/.tsxunless specific runtime requirements demand otherwise.Besides that, I think the logic of "so if someone create them, he might be more likely to realize those are wrong?" is flawed. They likely won't realize anything, starting with not realizing eslint is not running on their code 😄. Better is to apply the rules to any file that could contain javascript/typescript and if we need to enforce a specific extension we should use a specific rule for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Janpot If a developer can't run the tests he/she try to write in dev mode, I think he/she will notice that something is off.
For eslint specifically, yeah, agree, the ideal is to lint the file and have an eslint rule that error because the extension is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, why have them waste time reverse engineering the tests to find out they're using the wrong extension if the eslint plugin can tell them in context in the editor? 🤔 Not that there would be anything wrong with using .jsx instead of .js for JSX files, contrary, it would kind of make things easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 https://www.npmjs.com/package/eslint-plugin-filename-rules sounds great.
I think that the value of only having
.jsis about not having to think about extensions. Having to rename a file after refactoring its content and moving the code around is friction to change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we can't get around this friction in typescript. Finishing the typescript migration means imposing this friction across the codebase anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙂 Coincidentally, I'm just running into a problem that illustrates why this is harmful. Just trying to figure out why I have a failing test locally in vitest that doesn't seem to break in mocha. The following produces a failing test:
But when you try running the global command:
Whoops, no test is running. What happened? Somewhere along the way we started running the build script natively in node. The file was renamed to .mjs. The test runner only looks for .js, .ts, and .tsx. The author didn't see any tests break after running them and assumed they were good. They didn't realize the test stopped running altogether. If the test framework was configured to run on any encountered javascript file, nothing would have been broken.
It took me a while to realize the test wasn't running, my first instinct was "something fails in vitest that breaks in mocha".