[code-infra] `vitest` browser & jsdom modes #14508

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

Minimal changes to have charts tests working in browser mode
Charts are the smallest test suit though

Currently ignoring DateFnsV3 adapter tests in the browser due to vitest-dev/vitest#6483 they work normally in jsdom though.

Running tests

# all
pnpm vitest

# browser
pnpm vitest --project "browser/*"

# jsdom
pnpm vitest --project "jsdom/*"

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

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

Generated by 🚫 dangerJS against aaa574f

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

oliviertassinari · 2024-09-11T17:02:11Z

.github/workflows/vitest.yml

@@ -0,0 +1,51 @@
+name: Vitest


CircleCI please: https://www.notion.so/mui-org/code-infra-Migrate-out-of-CircleCI-42350363b7344380a9961cf9731aae31 for the final version of this effort.

Yeah, this is just for ensuring the changes work while working on them

Can we remove the GH workflow given that CircleCI is already setup? 🤔

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.

github-actions · 2025-02-23T10:27:46Z

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

…-browser-working

JCQuintas added test scope: code-infra Specific to the core-infra product 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

JCQuintas added 18 commits February 25, 2025 23:21

remove unnecessary override

46bcb69

skip test

c9fa5e1

skip tests

6f9e916

skip test

d1ac98d

skip all in browser

642e4ef

ignore all in browser

ca4eaba

remove parallelism

0f8c8cf

fix date pickers isolation issues

e9853e0

fix datagrid isolate

11fa02b

fix datepickers pro isolate

a352883

datagrid test isolate

1f80763

fix charts isolate

fe0b0d6

turn on isolate ci

edd2907

Merge commit '3697464969746f746e17342bf97cff79657dfd71' into vitest-x…

abcb953

…-browser-working

update packages

da00ea5

fix ci

001e933

fix test

dda8ecd

add telemetry tests

e17895d

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

JCQuintas added 10 commits February 26, 2025 17:35

try fixes

f87f521

try closing dialog after test

66dc80b

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

36664df

remove changes

7b7c193

fix matchmedia

45c6a1a

m+

533665b

increase timeout for tests

701bd9c

increase worker

d34359c

decrease workers

aaa574f

revert

9038bed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[code-infra] `vitest` browser & jsdom modes #14508

[code-infra] `vitest` browser & jsdom modes #14508

JCQuintas commented Sep 5, 2024 •

edited

Loading

mui-bot commented Sep 5, 2024 •

edited

Loading

github-actions bot commented Sep 9, 2024

github-actions bot commented Sep 11, 2024

JCQuintas commented Sep 11, 2024

oliviertassinari Sep 11, 2024

oliviertassinari Sep 11, 2024

JCQuintas Sep 11, 2024

LukasTy Nov 7, 2024

oliviertassinari left a comment

oliviertassinari Sep 18, 2024 •

edited

Loading

JCQuintas Oct 4, 2024

oliviertassinari Oct 16, 2024

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

Janpot Oct 17, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024

JCQuintas commented Oct 8, 2024

github-actions bot commented Oct 11, 2024

github-actions bot commented Feb 23, 2025

[code-infra] vitest browser & jsdom modes #14508

Are you sure you want to change the base?

[code-infra] vitest browser & jsdom modes #14508

Conversation

JCQuintas commented Sep 5, 2024 • edited Loading

Running tests

mui-bot commented Sep 5, 2024 • edited Loading

github-actions bot commented Sep 9, 2024

github-actions bot commented Sep 11, 2024

JCQuintas commented Sep 11, 2024

oliviertassinari Sep 11, 2024

Choose a reason for hiding this comment

oliviertassinari Sep 11, 2024

Choose a reason for hiding this comment

JCQuintas Sep 11, 2024

Choose a reason for hiding this comment

LukasTy Nov 7, 2024

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

JCQuintas Oct 4, 2024

Choose a reason for hiding this comment

oliviertassinari Oct 16, 2024

Choose a reason for hiding this comment

Janpot Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Janpot Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Janpot Oct 17, 2024

Choose a reason for hiding this comment

Janpot Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2024

JCQuintas commented Oct 8, 2024

github-actions bot commented Oct 11, 2024

github-actions bot commented Feb 23, 2025

[code-infra] `vitest` browser & jsdom modes #14508

[code-infra] `vitest` browser & jsdom modes #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