Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.7k
[code-infra] Bring batch of changes from vitest PR#46795
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
mui-bot commentedAug 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Netlify deploy previewhttps://deploy-preview-46795--material-ui.netlify.app/ Bundle size report
|
| awaitrenderSpeedDial(); | ||
| constscreen=awaitrenderSpeedDial(); |
oliviertassinariAug 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
This seems to go backward. Tests are supposed to use thescreen from the global import (not all of them do this yes, but we are trending toward it).
| awaitrenderSpeedDial(); | |
| constscreen=awaitrenderSpeedDial(); | |
| awaitrenderSpeedDial(); |
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.
I believe we should reverse that trend. Using globalscreen breaks the test isolation. I believe react testing library makes the assumptiondocument andwindow globals never get reassigned as illustrated bytesting-library/user-event#1295. There doesn't seem to be much motivation on their end to fix this, so I think it would be easiest to just avoid the globalscreen altogether.
oliviertassinariAug 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Using global screen breaks the test isolation.
I wouldn't expect this to be the case though. Is this about a bug?
I believe react testing library makes the assumption document and window globals never get reassigned as illustrated
Their docs uses the global API:https://testing-library.com/docs/react-testing-library/example-intro#quickstart. Are we supposed to reassign document and window? This sounds slower than it needs to be. We used to share the same document/window between all the tests:
material-ui/test/utils/setup.js
Line 6 in24e561c
| createDOM(); |
We would only unmount the component withhttps://testing-library.com/docs/react-testing-library/api/#cleanup. If there was a leak, this would break other tests, which was great as it would force us to fix the component (it would leak equally in our users, unless the leak is in a pourly written test, but then I guess we can assert that the DOM is clean and have a flew other checks). I imagine that since the tests also run in the browser, we can't even afford to reset document and window in that context, so we have to work with them being the same.
Them having this issue could suggest that our setup is not standard.
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.
Them having this issue could suggest that our setup is not standard.
Correct, our setup is non-standard vitest. Standard vitest is one suite, one process. This turned out about twice as slow as mocha. Mocha runs in a single process and creates the dom only once for all suites. It's possible to run vitest in a single process--no-isolate --no-file-parallelism, but it'll still recreates the dom for each suite. You may find this counterintuitive, but I don't expect this to be a very heavy operation. As is proven here, the vitest setup in this mode runs at a similar speeds to mocha.
This sounds slower than it needs to be.
Maybe a bit, keep in mind that loading thejsdom library is magnitudes slower than instantiating it:
console.time('load-jsdom');const{JSDOM}=require('jsdom');console.timeEnd('load-jsdom');console.time('instantiate-jsdom');constdom=newJSDOM(`<!DOCTYPE html><p>Hello world</p>`);console.timeEnd('instantiate-jsdom');
gives the following onStackBlitz
load-jsdom: 265.355msinstantiate-jsdom: 19.59msso when running isolate processes for our 500 suites, that is (265ms + 20ms) * 500 = 142.5s spent on jsdom, for running in a single process and recreating dom for each suite is 265ms + (500 * 20ms) = 10.3s, so technically it's a bit slower than it needs to be (vs mocha's 520ms), but it's 14 times more efficient than vitest in standard mode.
Loading libraries is an often overlooked aspect of performance, but for us, with 500+ suites it can be significant. e.g. I'm replacinglodash withlodash.kebabcase. Which is about 50ms to load vs 3ms. This becomes seriously significant when running vitest in parallel mode, it's essentially spending 25s on loading lodash, just for a single function.
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.
Actually, just did another test:
console.time('load-jsdom');const{JSDOM}=require('jsdom');console.timeEnd('load-jsdom');console.time('instantiate-jsdom');constdom=newJSDOM(`<!DOCTYPE html><p>Hello world</p>`);console.timeEnd('instantiate-jsdom');console.time('instantiate-jsdom-2');constdom2=newJSDOM(`<!DOCTYPE html><p>Hello world</p>`);console.timeEnd('instantiate-jsdom-2');
Gives:
load-jsdom: 262.32msinstantiate-jsdom: 20.845msinstantiate-jsdom-2: 3.81msTurns out that instantiating when it's warm drops the time by another magnitude. instantiating jsdom becomes super cheap. So my 265ms + (500 * 20ms) = 10.3s is wrong. It should be 265ms + 20ms + 499*3ms = 1.7s. Negligible in our setup
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.
Ok, reverting the screen update for now to get the other changes in
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.
Ok, reverting the screen update for now to get the other changes in
Example of people migrating from container to screen:https://github.com/mui/base-ui/pull/2631/files#diff-7dd458b9ee88f78d6f4d7ac560f5b7765bf4b6814002411c663e3cd4fec33f0fR1801
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.
Probably not worth it migrating all instances of this on the core repo, there are a lot and nothing is to be gained from it in terms of helping with the vitest migration.
29f37dc intomui:masterUh oh!
There was an error while loading.Please reload this page.
| importformatUtilfrom'format-util'; | ||
| import_from'lodash'; | ||
| // avoid loading whole lodash, it takes ~50ms to initialize | ||
| importkebabCasefrom'lodash.kebabcase'; |
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.
Nitpick: This broke the package.
Bumping to the latest version (2.0.14) on thebase-ui repo makes all tests fail sincelodash.kebabcase is not in the list of dependencies.
Not to worry, since with the newest release the usages are replaced withes-toolkit.
Uh oh!
There was an error while loading.Please reload this page.
Trying to make#44325 more maintainable by moving all the test updates and fixes to its own PR
remove globalscreen, this breaks test isolation in vitestpackages/mui-codemod/src/v5.0.0/tests, vitest equivalent of require.context doesn't deal well with multiple nesting levelsglobalwithglobalThis