[code-infra] Replace `mocha` with `vitest` for browser & jsdom tests #14508

JCQuintas · 2024-09-05T18:42:18Z

Running tests

# jsdom
pnpm test
pnpm text:jsdom
pnpm test:unit:jsdom

# browser
pnpm test:browser
pnpm test:unit:browser

## Filter projects

### starting with `x-charts`
... --project "x-charts*"

### exactly `x-data-grid`
... --project "x-data-grid"

### ending with `-pro`
... --project "*-pro"

Work

Moved --coverage to V8/C8, and moved it to browser instead of jsdom as it was faster
Removed usage of clock='fake' and uses the vitest provided versions throughout the application.
- The goal is to eventually create a proper handler for it to replicate our most used pattern
- Issue with previous implementation is that it required faking the clock/timers for ALL tests in a suit, rather than just the ones that needed it.
Vitest config at
- packages/**/vitest.config.browser.mts
- packages/**/vitest.config.jsdom.mts
skipLibCheck=true added to some packages due to chai issues with @mui/internal-test-utils

Times

JSDOM: 11-13m
BROWSER: 5-6m

Table is a bit outdated, times are faster now (above). Left it as a comparison.

class	runner	time browser	time jsdom	coverage	pool
medium	mocha	8m	14m	JSDOM	*
medium	vitest	9m	19m	JSDOM	fork
medium	vitest	13m	15m	BROWSER	fork
medium+	vitest	7m	19m	JSDOM	fork
large	vitest	6m	17m	JSDOM	fork

mui-bot · 2024-09-05T23:49:47Z

Deploy preview: https://deploy-preview-14508--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 73c6a59

github-actions · 2024-09-09T10:28:15Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2024-09-11T09:34:31Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

JCQuintas · 2024-09-11T16:50:06Z

@LukasTy @flaviendelangle @noraleonte @arthurbalduini I think I've fixed most of the issues with date-pickers in vitest that I could without going super deep on how everything works 😅

I now need some help with understanding the current errors, and possible ideas on how to fix them.

You can check the current failing tests here:
Vitest Tests (jsdom)
Vitest Tests (browser)

packages/x-charts/src/ChartsTooltip/contentDisplayed.test.tsx

oliviertassinari · 2024-09-11T17:00:35Z

vitest.workspace.ts

+        browser: {
+          enabled: true,
+          name: 'chromium',
+          provider: 'playwright',
+          headless: true,
+          // https://playwright.dev
+          providerOptions: {},
+          screenshotFailures: false,
+        },


Interesting

.github/workflows/vitest.yml

oliviertassinari

The speed of the tests in the CI looks encouraging 👍 . A bit early to see if there is a real difference with the current CI though (test coverage vs. none, CircleCI vs. GitHub Action).

oliviertassinari · 2024-09-18T16:08:27Z

.eslintrc.js

@@ -37,7 +37,7 @@ const addReactCompilerRule = (packagesNames, isEnabled) =>
  !isEnabled
    ? []
    : packagesNames.map((packageName) => ({
-        files: [`packages/${packageName}/src/**/*{.ts,.tsx,.js}`],
+        files: [`packages/${packageName}/src/**/*.?(c|m)[jt]s?(x)`],


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.

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.

but less complete.

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?

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?

Quite the contrary, we sometimes need those extensions:

When running scripts natively on node (or through tsx), we need a way to signal node which module system is being used.

We need .cjs in edge-cases for upcoming ESM support in places where we want to bridge between ESM and CJS, e.g. to deal with importing next/document in ESM.

Tooling like esbuild, tsx and vitest rely 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 .js files. See Enable JSX in .js files 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 .ts files to adhere to the JSX rules, which required me to refactor some code.

If we want our output to be interpreted correctly as ESM in node, it will need the correct extension. (granted, this code is not authored and thus not linted)

...

We shouldn't restrict extensions, they're not for cosmetic reasons. We should just prefer .ts/.tsx unless 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.

They likely won't realize anything

@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.

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.

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.

if the eslint plugin can tell them in context in the editor?

👍 https://www.npmjs.com/package/eslint-plugin-filename-rules sounds great.

Not that there would be anything wrong with using .jsx instead of .js for JSX files

I think that the value of only having .js is 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.

But we can't get around this friction in typescript. Finishing the typescript migration means imposing this friction across the codebase anyway.

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.

🙂 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:

pnpm --filter @mui/icons-material test -- -g "\"should produce the expected output\""

But when you try running the global command:

pnpm test:coverage -- -g "\"should produce the expected output\""

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".

.mocharc.js

vitest.workspace.ts

github-actions · 2024-09-19T10:56:57Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

JCQuintas · 2024-10-08T14:18:31Z

@LukasTy had to move away from global overrides in favour of vi.mock because the later works correctly and with minimal required changes in both jsdom and browser. We just have to mock the mock function when running in mocha 😅

Changes are here:
2d5d395

github-actions · 2024-10-11T22:15:01Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

packages/x-date-pickers/vitest.config.jsdom.mts

packages/x-data-grid/vitest.config.browser.mts

Janpot

Looks good overall, a few reflections

The project naming and filtering logic feel quite complicated to me. Are there no ways to simplify. i.e. is it not possible with wildcards? e.g. --project=jsdom/* or --project=*/@mui/x-charts. I could see it making harder to build more tooling around this if projects don't have a static name.
Either I misunderstand it or the date-fns resolution plugin could use resolution apis and not code transformation.

I wouldn't be against creating a vitest config factory e.g.

export default createConfig({ browser: true, hideScrollbars: true })

LukasTy

A couple of additional comments/issues:

I think we should replace mocha in tsconfig types with vitest (in package tests), since now all describe and it calls still point to mocha.
It looks like adding .only to a block (i.e. describe) does not limit test running only to that particular block.
IMHO, this is a blocker. 🙈

JCQuintas · 2025-05-05T13:41:48Z

It looks like adding .only to a block (i.e. describe) does not limit test running only to that particular block.

This is a behaviour of vitest 🫠

Jest has the same issue: jestjs/jest#4414

The docs recommend using npm vitest FileName to filter for a single file
https://vitest.dev/api/#describe-only

Or run it in watch-mode which will run on changed files only

test/utils/pickers/describePicker/describePicker.tsx

LukasTy · 2025-05-05T14:53:47Z

This is a behaviour of vitest 🫠

Jest has the same issue: jestjs/jest#4414

The docs recommend using npm vitest FileName to filter for a single file https://vitest.dev/api/#describe-only

That's "slightly" annoying. 🙈
What if I want to run certain only blocks in multiple files? 🤔
I will need to include all the files? 🙈 🤷
In any case, it seems that we got used to comfortable single-threaded features... 🙈

…-browser-working

JCQuintas · 2025-05-06T21:35:36Z

I wouldn't be against creating a vitest config factory e.g.
export default createConfig({ browser: true, hideScrollbars: true })

I'm not against it, but I would rather do it in a future step if necessary.

I think the current way works, although it is a bit wordy. We already have a place to store shared config in vitest.shared.mts, so the changes would to have a createConfig to mostly automate this:

  test: {
    name: getTestName(import.meta.url),
    environment: 'browser',
    browser: {
      enabled: true,
      instances: [
        {
          browser: 'chromium',
        },
      ],
    },
  },

…-browser-working

test/vite-plugin-filter-replace.mts

Co-authored-by: Jan Potoms <[email protected]> Signed-off-by: Jose C Quintas Jr <[email protected]>

…-browser-working

LukasTy

LGTM. 👍
Thank you for all the massive effort working on this! 🙏 💯 💙

JCQuintas added test scope: code-infra labels Sep 5, 2024

JCQuintas self-assigned this Sep 5, 2024

JCQuintas mentioned this pull request Sep 5, 2024

[code-infra] Changes for test util to work in vitest mui/material-ui#43625

Merged

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 9, 2024

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 10, 2024

github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 11, 2024

JCQuintas changed the title ~~[code-infra] vitest browser mode working - charts~~ [code-infra] vitest browser mode Sep 11, 2024

JCQuintas changed the title ~~[code-infra] vitest browser mode~~ [code-infra] vitest browser & jsdom modes Sep 11, 2024

JCQuintas mentioned this pull request Sep 11, 2024

[code-infra] Support parallel tests mui/mui-public#170

Open

oliviertassinari reviewed Sep 11, 2024

View reviewed changes

oliviertassinari reviewed Sep 18, 2024

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 19, 2024

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 8, 2024

This was referenced Oct 8, 2024

[code-infra] Remove deprecated data-mui-test in favour of data-testid #14882

Merged

[code-infra] Prepare some tests to work in vitest/playwright #14926

Merged

github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 11, 2024

Janpot reviewed May 5, 2025

View reviewed changes

packages/x-date-pickers/vitest.config.jsdom.mts Outdated Show resolved Hide resolved

Janpot reviewed May 5, 2025

View reviewed changes

packages/x-data-grid/vitest.config.browser.mts Show resolved Hide resolved

Janpot reviewed May 5, 2025

View reviewed changes

LukasTy reviewed May 5, 2025

View reviewed changes

test/utils/pickers/describePicker/describePicker.tsx Outdated Show resolved Hide resolved

JCQuintas added 3 commits May 6, 2025 21:23

Merge commit 'c25066404f6e0ed4a02ebc63d483597894a29c46' into vitest-x…

ce34df4

…-browser-working

suggestions

8b35b39

re-enable pickers test that was failing

65fe98d

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 6, 2025

JCQuintas added 4 commits May 6, 2025 23:37

reverts

f158e0e

Merge commit '53fd0e32860985c6629346ffd844dd674a78ddb1' into vitest-x…

2877d64

…-browser-working

vitest redirect imports

ef3a9bd

exclude -> include

fb13349

Janpot reviewed May 7, 2025

View reviewed changes

test/vite-plugin-filter-replace.mts Outdated Show resolved Hide resolved

JCQuintas and others added 9 commits May 7, 2025 14:50

Update test/vite-plugin-filter-replace.mts

b87e324

Co-authored-by: Jan Potoms <[email protected]> Signed-off-by: Jose C Quintas Jr <[email protected]>

move includes to prop of config

46a2b70

change to single include and document

b6d49c6

add /locale back

cd8161d

formatting

fce7e7a

Merge commit 'c3fe709fe61282c152fbbdc0bc812080ec6e9fc3' into vitest-x…

dc5e792

…-browser-working

prettier

b981102

simple projects names

480de46

Merge branch 'master' into vitest-x-browser-working

73c6a59

LukasTy approved these changes May 8, 2025

View reviewed changes

JCQuintas mentioned this pull request May 8, 2025

[code-infra] Replace mocha types to vitest types #17751

Open

JCQuintas merged commit e2dceb2 into mui:master May 8, 2025
23 checks passed

JCQuintas deleted the vitest-x-browser-working branch May 8, 2025 13:46

Uh oh!

[code-infra] Replace mocha with vitest for browser & jsdom tests #14508

[code-infra] Replace mocha with vitest for browser & jsdom tests #14508

Uh oh!

Conversation

JCQuintas commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Running tests

Work

Times

Uh oh!

mui-bot commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 9, 2024

Uh oh!

github-actions bot commented Sep 11, 2024

Uh oh!

JCQuintas commented Sep 11, 2024

Uh oh!

Uh oh!

oliviertassinari Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oliviertassinari left a comment

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JCQuintas Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

Janpot Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Janpot Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Janpot Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

Janpot Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 19, 2024

Uh oh!

JCQuintas commented Oct 8, 2024

Uh oh!

github-actions bot commented Oct 11, 2024

Uh oh!

Uh oh!

Uh oh!

Janpot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukasTy left a comment

Choose a reason for hiding this comment

Uh oh!

JCQuintas commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

[code-infra] Replace `mocha` with `vitest` for browser & jsdom tests #14508

[code-infra] Replace `mocha` with `vitest` for browser & jsdom tests #14508

JCQuintas commented Sep 5, 2024 •

edited

Loading

mui-bot commented Sep 5, 2024 •

edited

Loading

oliviertassinari Sep 18, 2024 •

edited

Loading

Janpot Oct 17, 2024 •

edited

Loading

oliviertassinari Oct 17, 2024 •

edited

Loading

Janpot Oct 17, 2024 •

edited

Loading

oliviertassinari Oct 17, 2024 •

edited

Loading

Janpot Oct 17, 2024 •

edited

Loading

Janpot left a comment •

edited

Loading

JCQuintas commented May 5, 2025 •

edited

Loading