[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/*"

…-browser-working

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

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

Generated by 🚫 dangerJS against 98013ae

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

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

…-browser-working

JCQuintas

With these last changes, ALL X packages are running in vitest.

There are some details I commented below.

Main takes:

x-license and x-internals are only running in jsdom mode.
x-date-pickers Hijiri adapter is only running in jsdom mode.

I can isolate the this.skip changes if necessary.

Tomorrow I'll check if I can make circle-ci work with vitest (I didn't try yet). So we can have proper comparisons

JCQuintas · 2024-10-14T20:09:35Z

package.json

+    "@types/node": "^20.16.10",
+    "@playwright/test": "1.44.1",
+    "playwright": "1.44.1",
+    "@mui/internal-test-utils": "https://pkg.csb.dev/mui/material-ui/commit/92c23999/@mui/internal-test-utils"


This needs to be changed when mui/material-ui#43625 is merged.

I would still leave it as a resolution so all packages use the same.

Personally, I would avoid all resolutions if we can, it tends to be misunderstood and overused and could result into setups that break on the user end but not on our end. (probably not for this dependency though)

If the version ranges are equal, there should be no duplication. And it shouldn't break in the first place if there was duplication.

JCQuintas · 2024-10-14T20:12:50Z

packages/x-charts/src/BarChart/checkClickEvent.test.tsx

+    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
-        this.skip();
+        // @ts-expect-error to support mocha and vitest
+        // eslint-disable-next-line @typescript-eslint/no-unused-expressions
+        this?.skip?.() || t?.skip();


You will see this pattern across multiple files, t = {} tells mocha to treat this as an "empty" parameter, thus not triggering the logic to provide the done() callback to it.

Without this mocha will fail because it waits for the done() to be called.

This can then be migrated to just a .skip, but vitest offers many ways of skipping test.

function test(t) { if (isJSDOM) { t.skip(); } }

I added a shim in ./setupVitest that we cold use while the tests run in both vitest and mocha. That prevents us in core from having to update all of these right now:
https://github.com/mui/material-ui/pull/43625/files#diff-21f1d3337bd5d6e3869ad10c93fa99770a34761876017b90bc1ed1ceaa7c93a9R45-R55
Want to move this to it.skipIf eventually

The same couldn't be done for describe.skipIf that only exists in vitest, I added a shim for that in test utils:

https://github.com/mui/material-ui/pull/43625/files#diff-52e69abe336786febd0b84dbd2d9b32ba0e8f74fcf5f71669c8917d9a434c4dcR3-R9

Don't necessarily have to use it, but it can keep the amount of code changes in tests down.

JCQuintas · 2024-10-14T20:14:35Z

packages/x-charts/src/BarChart/checkClickEvent.test.tsx

+  beforeEach(() => {
+    if (window?.document?.body?.style) {
+      window.document.body.style.margin = '0';
+    }
+  });
+
+  afterEach(() => {
+    if (window?.document?.body?.style) {
+      window.document.body.style.margin = '8px';
+    }
+  });


Some charts tests are reliant screen position, and margins are different for karma and vitest, so we force one. 🙃

JCQuintas · 2024-10-14T20:18:31Z

packages/x-data-grid-premium/src/tests/cellSelection.DataGridPremium.test.tsx

+      // fireEvent.mouseDown(getCell(2, 0), { ctrlKey: true });
+      // fireEvent.mouseOver(getCell(3, 0), { ctrlKey: true });
+      const isMac = window.navigator.platform.toUpperCase().indexOf('MAC') >= 0;
+
+      await user.keyboard(isMac ? '{Meta>}' : '{Control>}');
+      await user.pointer([
+        // touch the screen at element1
+        { keys: '[MouseLeft>]', target: getCell(2, 0) },
+        // move the touch pointer to element2
+        { target: getCell(3, 0) },
+        // release the touch pointer at the last position (element2)
+        { keys: '[/MouseLeft]' },
+      ]);
+      await user.keyboard(isMac ? '{/Meta}' : '{/Control}');


Only the userEvents worked here. Gotta change from meta/control based on OS.

JCQuintas · 2024-10-14T20:20:23Z

packages/x-data-grid-premium/src/tests/clipboard.DataGridPremium.test.tsx

+      // These test are flaky in JSDOM
+      return;


Early returns, we should eventually remove these into native skip describes to make tests more reliable.

I didn't find an api that worked for both mocha and vitest except for returning.

Added a describeSkipIf to test utils that shims vitest API of describe.skipIf

https://github.com/mui/material-ui/pull/43625/files#diff-52e69abe336786febd0b84dbd2d9b32ba0e8f74fcf5f71669c8917d9a434c4dcR3-R9

JCQuintas · 2024-10-14T20:36:05Z

packages/x-date-pickers/src/DesktopDatePicker/tests/DesktopDatePicker.test.tsx

+      act(() => {
+        screen.getByLabelText(/choose date/i).click();
+      });


Wrap in act, .click() so it doesn't scroll into view.

JCQuintas · 2024-10-14T20:43:08Z

test/utils/pickers/describeRangeValidation/testTextFieldKeyboardRangeValidation.tsx

      });

      expect(onErrorMock.callCount).to.equal(2);
      expect(onErrorMock.lastCall.args[0]).to.deep.equal(['disablePast', 'disablePast']);
      testInvalidStatus([true, true], isSingleInput);

      act(() => {
-        setValue(adapterToUse.date(now));
+        setValue(adapterToUse.addHours(now, 1));


Had to add one hour to "now" for it to work here. I guess previously the timer didn't run?

JCQuintas · 2024-10-14T20:44:43Z

vitest.workspace.ts

+// Ideally we move the configuration to each package.
+// Currently it doesn't work because vitest doesn't detect two different configurations in the same package.
+// We could bypass this limitation by having a folder per configuration. Eg: `packages/x-charts/browser` & `packages/x-charts/jsdom`.


// comment

JCQuintas · 2024-10-14T21:03:21Z

vitest.workspace.ts

+  // TODO: Decide on
+  // Manually changing the process.env in browser tests doesn't work.
+  // And alternative is to use `const {NODE_ENV} = process.env` in the code instead.
+  // x-license relies on `process.env.NODE_ENV` to determine the environment.
+  {
+    extends: './vitest.config.mts',
+    plugins: [react()],
+    test: {
+      include: [`packages/x-license/src/**/*.test.?(c|m)[jt]s?(x)`],
+      exclude: [`packages/x-license/src/**/*.browser.test.?(c|m)[jt]s?(x)`],
+      name: `jsdom/x-license`,
+      environment: 'jsdom',
+      env: {
+        MUI_JSDOM: 'true',
+      },
+    },
+  },


x-license required to be run on jsdom only due to the process.env requirement.

JCQuintas · 2024-10-14T22:06:23Z

vitest.workspace.ts

+  // TODO: Decide on
+  // Manually changing the process.env in browser tests doesn't work.
+  // And alternative is to use `const {NODE_ENV} = process.env` in the code instead.
+  // x-license relies on `process.env.NODE_ENV` to determine the environment.
+  // x-internals has a weird chai import issue I couldn't fix
+  ...jsdomOnlyPackages.map((name) => ({
+    extends: './vitest.config.mts',
+    plugins: [react()],
+    test: {
+      include: [`packages/${name}}/src/**/*.test.?(c|m)[jt]s?(x)`],
+      exclude: [`packages/${name}}/src/**/*.browser.test.?(c|m)[jt]s?(x)`],
+      name: `jsdom/${name}}`,
+      environment: 'jsdom',
+      env: {
+        MUI_JSDOM: 'true',
+      },
+    },
+  })),


Running these on jsdom only as x-license requires process.env.NODE_ENV and x-internals has a weird chai error I couldn't fix.

JCQuintas · 2024-10-15T10:02:13Z

.circleci/config.yml

+  test_unit_vitest:
+    <<: *default-job
+    steps:
+      - checkout
+      - install_js
+      - run:
+          name: Tests fake browser (vitest)
+          command: pnpm vitest --project "jsdom/*" --coverage
+      - run:
+          name: Check coverage generated
+          command: |
+            if ! [[ -s coverage/lcov.info ]]
+            then
+              exit 1
+            fi


@Janpot is it possible to test circleci steps without merging? 🤔

Should be yes, when you add it to the workflow:

mui-x/.circleci/config.yml

Line 332 in 98013ae

jobs:

…-browser-working

Janpot · 2024-10-16T09:10:11Z

.mocharc.js

@@ -7,6 +8,8 @@ module.exports = {
    // Mocha seems to ignore .next anyway (maybe because dotfiles?).
    // We're leaving this to make sure.
    'docs/.next/**',
+    'packages/**/*.browser.test.{js,ts,tsx,jsx}',


Is this a new file extension we're introducing? Can't find any such file.

Janpot · 2024-10-16T09:10:57Z

babel.config.js

@@ -106,17 +106,17 @@ module.exports = function getBabelConfig(api) {
    plugins.push([


Let's add a TODO here to remove these plugins once fully migrated to vitest?

Janpot · 2024-10-16T09:30:45Z

vitest.workspace.ts

+    // And alternative is to use `const {NODE_ENV} = process.env` in the code instead.
+    // x-license relies on `process.env.NODE_ENV` to determine the environment.
+    // x-internals has a weird chai import issue I couldn't fix
+    ...(jsdomOnlyPackages.includes(name)


It feels suboptimial to me to have to define this on the workspace level and not on the individual package level.
Would it be possible to give each vitest project their own config file which extends from one or more base files?

For instance we could just define workspaces as

// ./vitest.workspace.ts export default [ './packages/*/vitest.config.{browser|jsdom}.ts' ]

and in the projects:

// ./packages/x-data-grid/vitest.config.browser.ts import { mergeConfig } from 'vitest/config' import browserDefault from '@mui/test-utils/browserDefault' export default mergeConfig(browserDefault, { // ... }) // ./packages/x-data-grid/vitest.config.jsdom.ts import jsdomDefault from '@mui/test-utils/jsdomDefault' export default jsdomDefault

and omit the browser file for projects that are jsdom only

Not sure whether that works the way I expect it to be

github-actions · 2024-10-16T10:11:51Z

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

Janpot · 2024-10-18T09:01:16Z

.eslintrc.js

@@ -244,7 +244,7 @@ module.exports = {
      },
    },
    {
-      files: ['packages/*/src/**/*{.ts,.tsx,.js}'],
+      files: ['packages/*/src/**/*.?(c|m)[jt]s?(x)'],
      excludedFiles: ['*.d.ts', '*.spec.ts', '*.spec.tsx'],


I propose as a pattern we use ?(c|m)[jt]s?(x) to select files, but use wildcard to exclude.

Suggested change

excludedFiles: ['*.d.ts', '*.spec.ts', '*.spec.tsx'],

excludedFiles: ['*.d.ts', '*.spec.*'],

Janpot · 2024-10-18T09:02:14Z

.eslintrc.js

@@ -58,7 +58,7 @@ const RESTRICTED_TOP_LEVEL_IMPORTS = [
 // It needs to know about the parent "no-restricted-imports" to not override them.
 const buildPackageRestrictedImports = (packageName, root, allowRootImports = true) => [
  {
-    files: [`packages/${root}/src/**/*{.ts,.tsx,.js}`],
+    files: [`packages/${root}/src/**/*.?(c|m)[jt]s?(x)`],
    excludedFiles: ['*.d.ts', '*.spec.ts', '*.spec.tsx', '**.test.tx', '**.test.tsx'],


"test.tx" 🙂

Suggested change

excludedFiles: ['*.d.ts', '*.spec.ts', '*.spec.tsx', '**.test.tx', '**.test.tsx'],

excludedFiles: ['*.d.ts', '*.spec.*', '*.test.*'],

vitest browser mode working charts

eff3bca

JCQuintas added test scope: code-infra Specific to the core-infra product labels Sep 5, 2024

JCQuintas self-assigned this Sep 5, 2024

Merge commit 'cc2a9a3669a362d7fe17e555a74f62ebb401f2ca' into vitest-x…

bd11253

…-browser-working

JCQuintas mentioned this pull request Sep 5, 2024

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

Open

JCQuintas added 2 commits September 6, 2024 01:34

Fix mocha + vitest

9a6cbf8

push runnable change

4a27d05

JCQuintas added 4 commits September 6, 2024 10:41

mocha vs vitest

fb7c2de

Remove playwright direct dep

118a532

try normalizing container

33293ca

try clearing the body margin

d14ad46

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

JCQuintas added 2 commits September 10, 2024 13:15

Merge commit 'c25374458e4a5f7f244494accc9666a6a9c1d1d8' into vitest-x…

5a67c1b

…-browser-working

Fix new tests

d39f9e4

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

JCQuintas added 12 commits September 10, 2024 15:06

fix jsdom

aa6b60d

browser and dom together

1db9bbb

packages

3e5caf5

add workflow

a1234b6

fix playwright install

9a6eb21

check permission

09de9d7

try removing duplicate dep

91c67c0

upgrade playwright

94bbb95

dedupe

8d37f26

Fix chai assertions

e47aa21

revert playwright

7f19f37

overwrite playwright in internal packages

ad28046

fix treeview

a1feb9b

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

JCQuintas added 6 commits October 14, 2024 21:57

enable treeview pro

3615198

rename setup file

8757aa4

update mui-internals and dedupe

9799824

fix tests

ed6dbb8

update comments

7c62bfc

jsdom only packages

4f6b9c4

JCQuintas commented Oct 14, 2024

View reviewed changes

JCQuintas requested review from Janpot, flaviendelangle, michaldudak, LukasTy, cherniavskii and alexfauquette October 14, 2024 22:13

JCQuintas added 2 commits October 15, 2024 11:39

setup circleci and enable coverage

5442abd

dedupe

aba0b63

JCQuintas commented Oct 15, 2024

View reviewed changes

JCQuintas added 4 commits October 15, 2024 13:00

make helper libs also testable under vitest

5a930f1

fix x-charts-vendor should import from esm

8066b23

Merge commit 'c1e882ce9832b2a52a9d620a1d8b70a3dbad3059' into vitest-x…

3bc1fa7

…-browser-working

run coverage in vitest gha

98013ae

Janpot reviewed Oct 16, 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 Oct 16, 2024

Janpot mentioned this pull request Oct 17, 2024

[code-infra] Fix icon builder tests mui/material-ui#44143

Merged

Janpot reviewed Oct 18, 2024

View reviewed changes

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

JCQuintas left a comment

JCQuintas Oct 14, 2024

Janpot Oct 16, 2024

JCQuintas Oct 14, 2024

Janpot Oct 16, 2024 •

edited

Loading

JCQuintas Oct 14, 2024

JCQuintas Oct 14, 2024

JCQuintas Oct 14, 2024

Janpot Oct 16, 2024

JCQuintas Oct 14, 2024

JCQuintas Oct 14, 2024

JCQuintas Oct 14, 2024

JCQuintas Oct 14, 2024

JCQuintas Oct 14, 2024

JCQuintas Oct 15, 2024

Janpot Oct 16, 2024

Janpot Oct 16, 2024

Janpot Oct 16, 2024

Janpot Oct 16, 2024

Janpot Oct 16, 2024

github-actions bot commented Oct 16, 2024

Janpot Oct 18, 2024

Janpot Oct 18, 2024

		@@ -106,17 +106,17 @@ module.exports = function getBabelConfig(api) {
		plugins.push([

	excludedFiles: ['.d.ts', '.spec.ts', '*.spec.tsx'],
	excludedFiles: ['.d.ts', '.spec.*'],

	excludedFiles: ['.d.ts', '.spec.ts', '.spec.tsx', '.test.tx', '*.test.tsx'],
	excludedFiles: ['.d.ts', '.spec.', '.test.*'],

[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

JCQuintas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[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

Janpot Oct 16, 2024 •

edited

Loading