[code-infra] Bring batch of changes from vitest PR #46795
Conversation
Netlify deploy previewhttps://deploy-preview-46795--material-ui.netlify.app/ Bundle size report
|
oliviertassinari
changed the title
| await renderSpeedDial(); | ||
| const screen = await renderSpeedDial(); |
There was a problem hiding this comment.
This seems to go backward. Tests are supposed to use the screen from the global import (not all of them do this yes, but we are trending toward it).
| await renderSpeedDial(); | |
| const screen = await renderSpeedDial(); | |
| await renderSpeedDial(); |
There was a problem hiding this comment.
I believe we should reverse that trend. Using global screen breaks the test isolation. I believe react testing library makes the assumption document and window globals never get reassigned as illustrated by testing-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 global screen altogether.
There was a problem hiding this comment.
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 in 24e561c
We would only unmount the component with https://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.
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 the jsdom library is magnitudes slower than instantiating it:
console.time('load-jsdom');
const { JSDOM } = require('jsdom');
console.timeEnd('load-jsdom');
console.time('instantiate-jsdom');
const dom = new JSDOM(`<!DOCTYPE html><p>Hello world</p>`);
console.timeEnd('instantiate-jsdom');gives the following on StackBlitz
load-jsdom: 265.355ms
instantiate-jsdom: 19.59ms
so 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 replacing lodash with lodash.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.
Actually, just did another test:
console.time('load-jsdom');
const { JSDOM } = require('jsdom');
console.timeEnd('load-jsdom');
console.time('instantiate-jsdom');
const dom = new JSDOM(`<!DOCTYPE html><p>Hello world</p>`);
console.timeEnd('instantiate-jsdom');
console.time('instantiate-jsdom-2');
const dom2 = new JSDOM(`<!DOCTYPE html><p>Hello world</p>`);
console.timeEnd('instantiate-jsdom-2');Gives:
load-jsdom: 262.32ms
instantiate-jsdom: 20.845ms
instantiate-jsdom-2: 3.81ms
Turns 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
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
Trying to make #44325 more maintainable by moving all the test updates and fixes to its own PR
screen, 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