[core] Make DatePickers tests independent from other packages #13951
Conversation
This reverts commit 9579c98.
JCQuintas
added
core
|
Deploy preview: https://deploy-preview-13951--material-ui-x.netlify.app/ |
| "private": true, | ||
| "scripts": { | ||
| "typescript": "tsc -p tsconfig.json", | ||
| "test:unit": "cross-env NODE_ENV=test TZ=UTC mocha -n expose_gc --config .mocharc.js" |
There was a problem hiding this comment.
When we were talking about this change, I had a slightly different picture in my head:
- Refactor utils if and how necessary
- Add package-specific test config (and mocha file) in the package itself
- Add the refactored packages into the root
mocharcfile to avoid running them via the root command - Add a functioning isolated
test:unitscript to transitioned packages - Augment the root
test:unitwith apnpm run test:unit -rto also run individual package tests
WDYT about that?
Did you consider such an approach?
Do you see problems with it? 🤔
I'm not the biggest fan of this direction as we'd end up with duplicate testing packages for each product line. 🙈
There was a problem hiding this comment.
My first approach was to have the config on each package. 😅
It works, but I felt it to be a bit clunky. x-date-pickers and x-date-pickers-pro use exactly the same config, so they would be duplicated or we would need a place to store them. Should they be in x-date-pickers? Felt a bit odd, as there might be necessary types/imports from -pro in some cases.
test/unit-date-pickers-> depends on@mui/x-date-pickers@mui/x-date-pickers-pro
This makes the tests dependant on the packages. Often times you will want to run the tests for them together anyways, so I felt this was fine.
The problem with utils folder is that it is a too generic name, so I would rather we move away from it in this use-case. We(as humans) tend to put anything there, so it acts as a container for everything, which is almost the same as no container at all. In general, folders that have a semantical meaning are easier to navigate, and should be preferred.
It is also important to note these changes need to be done step by step. As we need to keep CI and coverage working as we go 😅
So in short:
- Either duplicate config, or shared, so moved all config to a single place, which made easier to just have a single config.
- Wanted to have a specific place for the
date-pickerutils, moved them to same package. - Changes don't affect CI/coverage for now
There was a problem hiding this comment.
I'm not the biggest fan of this direction as we'd end up with duplicate testing packages for each product line. 🙈
I don't get what you mean with this. Which packages would be we duplicating?
There was a problem hiding this comment.
@LukasTy Correct me if I'm wrong, but you mean we will end up with the following internal packages
test/unit-date-pickerstest/unit-chartstest/unit-data-gridtest/unit-tree-view
About community/pro testing, if some pro types are needed to define some test, then you can test both pro and community future in the pro package
There was a problem hiding this comment.
Oh, sorry, I forgot to reply to this one. 🙈
Yes, exactly, I was mentioning the "duplication" or introduction of test/unit-<package> packages for each component family. 👍
I'm not against it, just had a different picture in my head initially and the proposed config is somewhat unconventional. 🤔
| @@ -25,6 +25,7 @@ const defaultAlias = { | |||
| ), | |||
| docs: resolveAliasPath('./node_modules/@mui/monorepo/docs'), | |||
| test: resolveAliasPath('./test'), | |||
| '@unit/date-pickers': resolveAliasPath('./test/unit-date-pickers'), | |||
There was a problem hiding this comment.
Nitpick: Do you see a problem with just sticking to the existing test alias? 🤔
I.e.: Have test/pickers/helpers or test/pickers/utils imports. 🤔
For now, the convention seems more or less clear:
- use
@muifor product packages and all external aliases - Use the folder name for other internal aliases (with the exception of
docs🙈 )
There was a problem hiding this comment.
I don't mind much about the naming. I just used what felt best.
testis generic so it didn't give much value, eg:test/date-pickerscould be confusing if we havee2e-date-pickersin the future.test/pickerscould be confusing if we have acolor-pickerwhich is a different package.
In general (contradictorily), I find the more precise the better 😅
So I felt @unit/date-pickers to be the most concise and gives all the info necessary. We can def change it to test/unit/date-pickers or test/pickers or anything else if you don't think the considerations above are relevant.
This is quite easy to refactor, we could change at any time if we decide to go with test/pickers and later we have something else there
oliviertassinari
added
the
component: pickers
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
removed
the
PR: out-of-date
JCQuintas
force-pushed
the
independent/pickers
branch
3 times, most recently
from
d24b835
to
e618a4e
Compare
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
JCQuintas
force-pushed
the
independent/pickers
branch
from
c092a90
to
2a5e3db
Compare
github-actions
bot
removed
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
added
the
PR: out-of-date
Related to #13882
test/unit-date-pickerswhich holds all config and helpers for date pickers unit testingtest/sharedfor any shared functionality that should happen in most/all teststest/, only dirstreeview|data-grid|chartstest configs into their own folders/scripts