[code-infra] Fix ESLint import restriction rule for test files #18669
Conversation
| `packages/*/src/**/*.test.${EXTENSION_TS}`, | ||
| `packages/*/src/**/*.spec.${EXTENSION_TS}`, |
There was a problem hiding this comment.
Addresses #18643 (comment).
eslint/config-airbnb-typescript.js
Outdated
| import { BarChartPro } from './BarChartPro'; | ||
|
|
||
| describe('<BarChartPro /> - License', () => { | ||
| const { render } = createRenderer(); | ||
|
|
||
| beforeEach(() => { | ||
| Object.keys(sharedLicenseStatuses).forEach((key) => { |
There was a problem hiding this comment.
setLicenseKey('') already clear license.
Did we need this?
It uses invalid import, which only works in "development" mode. 🤔
There was a problem hiding this comment.
setLicenseKey('') already clear license.
It clears the license, not the cache. If the cache persists between two or more files that test if the license is not present, then the error will not be logged.
It uses invalid import, which only works in "development" mode. 🤔
We are in test, testing a development feature, so I would expect that to be ok
There was a problem hiding this comment.
Should every test run in isolation and be devoid of caching? 🤔
To me it seems like our testing setup is slightly inefficient if we need to fight it like this to run a test without a license key being set. 🤔
There was a problem hiding this comment.
We can test, but IIRC isolation increases the test times by a considerable amount.
There was a problem hiding this comment.
At least locally, the difference is as follows when running CI=true pnpm test:unit.
So, around a 15% increase in runtime. 🤔
There was a problem hiding this comment.
So it technically should increase the CI time by about 2mins
There was a problem hiding this comment.
Our tests are slow as they are already. 🙈
I didn't want to increase the runtime even further. 🤷
Even though, I'd say, that in an ideal world, this repo should run tests with the default flag value.
|
Deploy preview: https://deploy-preview-18669--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: 🔺+362B(0.00%) - Total Gzip Change: 🔺+69B(0.00%) Show details for 100 more bundles (22 more not shown)@mui/x-charts/ChartsLabel parsed: 🔺+324B(+7.83%) gzip: 🔺+70B(+3.84%) |
| @@ -1,3 +1,4 @@ | |||
| export * from './ChartsLabel'; | |||
There was a problem hiding this comment.
Was the missing export a mistake?
Or is it intentional and I should prefer relative import or ESLint rule disabling? 🤔
There was a problem hiding this comment.
Confirming with @alexfauquette.
There was a problem hiding this comment.
Possibly a mistake
@JCQuintas You're the best person to say if it's a mistake or not since it's the labels of your PR to move the legend to HTML :)
I'm ok with exporting it. It' s a dumb component for which we already export classes
There was a problem hiding this comment.
Thank you for confirming. 🙏
Follow-up on #18643.