[test] Add `Charts` test #11551

alexfauquette · 2024-01-02T15:54:38Z

I was trying to add test to x-charts, but the ESM issue seems to be blocking.

The yarn test:unit returns

TypeError: Unknown file extension ".ts" for /home/alexandre/dev/mui/mui-x/packages/x-charts/src/LineChart/formatter.test.ts
    at new NodeError (node:internal/errors:393:5)
    at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:75:9)
    at defaultGetFormat (node:internal/modules/esm/get_format:114:38)
    at defaultLoad (node:internal/modules/esm/load:81:20)
    at nextLoad (node:internal/modules/esm/loader:161:28)
    at ESMLoader.load (node:internal/modules/esm/loader:594:26)
    at ESMLoader.moduleProvider (node:internal/modules/esm/loader:446:22)
    at new ModuleJob (node:internal/modules/esm/module_job:64:26)
    at ESMLoader.#createModuleJob (node:internal/modules/esm/loader:469:17)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:423:34)
error Command failed with exit code 1.

The yarn t formatter returns

Error [ERR_REQUIRE_ESM]: require() of ES Module /home/alexandre/dev/mui/mui-x/node_modules/d3-shape/src/index.js from /home/alexandre/dev/mui/mui-x/packages/x-charts/src/LineChart/formatter.ts not supported.
Instead change the require of index.js in /home/alexandre/dev/mui/mui-x/packages/x-charts/src/LineChart/formatter.ts to a dynamic import() which is available in all CommonJS modules.
    at Object.newLoader [as .js] (/home/alexandre/dev/mui/mui-x/node_modules/pirates/lib/index.js:141:7)
    at Object.<anonymous> (/home/alexandre/dev/mui/mui-x/packages/x-charts/src/LineChart/formatter.ts:8:16)
    at Module._compile (/home/alexandre/dev/mui/mui-x/node_modules/pirates/lib/index.js:136:24)
    at Object.newLoader [as .ts] (/home/alexandre/dev/mui/mui-x/node_modules/pirates/lib/index.js:141:7)
    at Object.<anonymous> (/home/alexandre/dev/mui/mui-x/packages/x-charts/src/LineChart/formatter.test.ts:4:41)
    at Module._compile (/home/alexandre/dev/mui/mui-x/node_modules/pirates/lib/index.js:136:24)
    at Object.newLoader [as .ts] (/home/alexandre/dev/mui/mui-x/node_modules/pirates/lib/index.js:141:7)
    at /home/alexandre/dev/mui/mui-x/node_modules/mocha/lib/mocha.js:414:36
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/home/alexandre/dev/mui/mui-x/node_modules/mocha/lib/mocha.js:411:14)
    at Mocha.run (/home/alexandre/dev/mui/mui-x/node_modules/mocha/lib/mocha.js:972:10)
    at Object.run (/home/alexandre/dev/mui/mui-x/node_modules/mocha/lib/cli/watch-run.js:263:22)
    at FSWatcher.<anonymous> (/home/alexandre/dev/mui/mui-x/node_modules/mocha/lib/cli/watch-run.js:184:14)

Does someone of the @mui/code-infra has an opinion on this topic. For now, the only solution I see is to write test with JS

mui-bot · 2024-01-02T15:58:50Z

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

Generated by 🚫 dangerJS against dfd79a4

Janpot · 2024-01-02T16:01:09Z

The first issue doesn't look ESM related to me, it seems like it's trying to run untranspiled typescript under node.js. Will take a look this week

oliviertassinari · 2024-01-02T17:11:53Z

packages/x-charts/src/LineChart/formatter.test.ts

+import { FormatterParams } from '@mui/x-charts/models/seriesType/config';
+import lineFormatter from '@mui/x-charts/LineChart/formatter';


These are private paths, so private APIs. I think importing with a relative path could make more sense:

Suggested change

import { FormatterParams } from '@mui/x-charts/models/seriesType/config';

import lineFormatter from '@mui/x-charts/LineChart/formatter';

import { FormatterParams } from '../seriesType/config';

import lineFormatter from './formatter';

Agree, I tried both to be sure the import wand not the cause of the issue

Janpot · 2024-01-04T14:18:17Z

Looks like I can get it to run with tsx. Haven't tested the impact on the rest of the test suite though:
See https://github.com/alexfauquette/mui-x/pull/11/files

.mocharc.js

Janpot

Minor typo in a comment. Otherwise looks good 👍

Co-authored-by: Jan Potoms <2109932+Janpot@users.noreply.github.com> Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

oliviertassinari

What's the rational to not apply the same fix to the data, grid and other components? (I mean why have two different mocha configurations?)

oliviertassinari · 2024-02-29T18:39:39Z

.circleci/config.yml

@@ -129,6 +129,9 @@ jobs:
    steps:
      - checkout
      - install_js
+      - run:
+          name: Tests charts
+          command: yarn test:charts:unit


Can we add a TODO to signal that we want to remove this once ESM issues are solved? It's weird to have different ways to run the tests.

Another option could be to have a test:unit on every package and then call workspace run test:unit in the root package test:unit, that way having a custom config for charts would be a bit less "hacky". 🙈
WDYT?

I prefer to keep the hacky feeling, just such that when reading this code it's obvious that something is wrong and need to be fixed (once ESM is done)

alexfauquette · 2024-03-01T12:53:10Z

What's the rational to not apply the same fix to the data, grid and other components?

Because with this mocha config the hack that swich between date-fns v2 and v3 is not applied, leading to errors in date picker tests

LukasTy

Because with this mocha config the hack that swich between date-fns v2 and v3 is not applied, leading to errors in date picker tests

That's what I feared the most when introducing that "hack". 🤔
I couldn't think of a better approach while still keeping the date-fns testing hack... 🤷
If we ever think of it - this hacky approach could be dropped. 🤞

P.S. I'm changing the PR title to "[test] Add Charts test" as it doesn't impact the package itself. 🤔

LukasTy · 2024-03-05T13:40:42Z

.circleci/config.yml

@@ -129,6 +129,9 @@ jobs:
    steps:
      - checkout
      - install_js
+      - run:
+          name: Tests charts
+          command: yarn test:charts:unit


Another option could be to have a test:unit on every package and then call workspace run test:unit in the root package test:unit, that way having a custom config for charts would be a bit less "hacky". 🙈
WDYT?

.mocharc.js

packages/x-charts/.mocharc.js

alexfauquette added test typescript component: charts This is the name of the generic UI component, not the React module! labels Jan 2, 2024

oliviertassinari reviewed Jan 2, 2024

View reviewed changes

Janpot mentioned this pull request Jan 4, 2024

One way to make the tests run alexfauquette/mui-x#11

Open

alexfauquette changed the title ~~[charts] Fix null in line chart using dataset~~ [charts] Add test Jan 8, 2024

alexfauquette mentioned this pull request Jan 8, 2024

[charts][ESM] @mui/x-charts does not work with jest #11568

Open

[charts] add unit test pipeline

e0e8435

alexfauquette force-pushed the add-test branch from d40a77f to e0e8435 Compare February 27, 2024 10:27

split script

ef61e88

alexfauquette marked this pull request as ready for review February 28, 2024 12:15

alexfauquette requested review from Janpot and LukasTy February 28, 2024 12:15

Janpot reviewed Feb 28, 2024

View reviewed changes

.mocharc.js Outdated Show resolved Hide resolved

Janpot approved these changes Feb 28, 2024

View reviewed changes

Update .mocharc.js

d95d76d

Co-authored-by: Jan Potoms <2109932+Janpot@users.noreply.github.com> Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

oliviertassinari reviewed Feb 29, 2024

View reviewed changes

LukasTy approved these changes Mar 5, 2024

View reviewed changes

LukasTy changed the title ~~[charts] Add test~~ [test] Add Charts test Mar 5, 2024

alexfauquette added 2 commits March 5, 2024 16:35

feedbacks

55ac296

Merge branch 'add-test' of github.com:alexfauquette/mui-x into add-test

dfd79a4

alexfauquette merged commit 2414dcf into mui:next Mar 7, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test] Add `Charts` test #11551

[test] Add `Charts` test #11551

alexfauquette commented Jan 2, 2024 •

edited

mui-bot commented Jan 2, 2024 •

edited

Janpot commented Jan 2, 2024

oliviertassinari Jan 2, 2024 •

edited

alexfauquette Jan 2, 2024

Janpot commented Jan 4, 2024

Janpot left a comment

oliviertassinari left a comment •

edited

oliviertassinari Feb 29, 2024

LukasTy Mar 5, 2024

alexfauquette Mar 5, 2024

alexfauquette commented Mar 1, 2024

LukasTy left a comment •

edited

LukasTy Mar 5, 2024

		import { FormatterParams } from '@mui/x-charts/models/seriesType/config';
		import lineFormatter from '@mui/x-charts/LineChart/formatter';

[test] Add Charts test #11551

[test] Add Charts test #11551

Conversation

alexfauquette commented Jan 2, 2024 • edited

mui-bot commented Jan 2, 2024 • edited

Janpot commented Jan 2, 2024

oliviertassinari Jan 2, 2024 • edited

Choose a reason for hiding this comment

alexfauquette Jan 2, 2024

Choose a reason for hiding this comment

Janpot commented Jan 4, 2024

Janpot left a comment

Choose a reason for hiding this comment

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

oliviertassinari Feb 29, 2024

Choose a reason for hiding this comment

LukasTy Mar 5, 2024

Choose a reason for hiding this comment

alexfauquette Mar 5, 2024

Choose a reason for hiding this comment

alexfauquette commented Mar 1, 2024

LukasTy left a comment • edited

Choose a reason for hiding this comment

LukasTy Mar 5, 2024

Choose a reason for hiding this comment

[test] Add `Charts` test #11551

[test] Add `Charts` test #11551

alexfauquette commented Jan 2, 2024 •

edited

mui-bot commented Jan 2, 2024 •

edited

oliviertassinari Jan 2, 2024 •

edited

oliviertassinari left a comment •

edited

LukasTy left a comment •

edited