-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[test] Use test-utils from npm #12880
Conversation
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Deploy preview: https://deploy-preview-12880--material-ui-x.netlify.app/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 👏
The failing tests are most likely not related to this PR, as they also fail when the monorepo is updated to I'd appreciate any assistance with these. |
This comment was marked as resolved.
This comment was marked as resolved.
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
mui/material-ui#41177 has been merged and @mui/internal-test-utils is published to npm with version 1.0.0-dev.20240529-082515-213b5e33ab |
@@ -6,7 +6,7 @@ | |||
"commitMessageTopic": "{{depName}}", | |||
"dependencyDashboard": true, | |||
"rebaseWhen": "conflicted", | |||
"ignoreDeps": ["jsdom"], | |||
"ignoreDeps": [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cherniavskii On the latest major they reverted a change that initially reduced the performance
https://github.com/jsdom/jsdom/releases/tag/24.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👏
Thanks for the effort. 💙
I've also updated jsdom
and ran dedupe
to reduce amount of duplicate packages, which were introduced for some reason. 🙈
Do you have any idea why argos reacted like there is no reference build and treats all screenshots as new? 🤔 🤷
It looks like an issue on Argos' side. I started seeing it in other PRs as well. |
@@ -32,7 +32,7 @@ | |||
"size:why": "pnpm size:snapshot --analyze --accurateBundles", | |||
"tc": "node test/cli.js", | |||
"test": "node scripts/test.mjs", | |||
"test:coverage": "cross-env NODE_ENV=test TZ=UTC BABEL_ENV=coverage nyc mocha --exclude '**/node_modules/**' && nyc report -r lcovonly", | |||
"test:coverage": "cross-env NODE_OPTIONS=--max-old-space-size=4096 NODE_ENV=test TZ=UTC BABEL_ENV=coverage nyc mocha --exclude '**/node_modules/**' && nyc report -r lcovonly", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LukasTy How did you manage to make the test_unit
pass? It failed even when I added this flag here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did it? I saw them passing even before bumping jsdom... 🤷
https://app.circleci.com/pipelines/github/mui/mui-x?branch=pull%2F12880
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I think I didn't wait for the result after my last commit, and for some reason assumed it failed 😅
All good then 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. Here is my comment comparing your commits behavior on the system resources. 😉
#12880 (comment)
@@ -48,6 +48,7 @@ | |||
}, | |||
"dependencies": { | |||
"@babel/runtime": "^7.24.5", | |||
"@mui/internal-test-utils": "1.0.0-dev.20240529-082515-213b5e33ab", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LukasTy this was added as a dependency, causing consumers to get this, and many others deps, installed.
this addition causes peerDep errors (about babel missing and react@18 not supported) on our end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for raising this! 🙏
An issue has also been created for it: #13317.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AviVahl A fixed package (v7.6.1) has been released: https://www.npmjs.com/package/@mui/x-data-grid?activeTab=versions
Co-authored-by: Andrew Cherniavskyi <andrew@mui.com> Co-authored-by: Lukas <llukas.tyla@gmail.com>
Co-authored-by: Andrew Cherniavskyi <andrew@mui.com> Co-authored-by: Lukas <llukas.tyla@gmail.com>
Using test-utils from an npm package instead of monorepo.
A couple of additional changes were needed:
moduleResolution
was set tobundler
to enable reading package.jsonexports
fieldTo be merged after mui/material-ui#41177 is merged and published.