Skip to content
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] Do not require ResizeObserver mock in JSDOM #8961

Closed
wants to merge 1 commit into from

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented May 11, 2023

TODO:

  • rebase on master

@cherniavskii cherniavskii added test component: data grid This is the name of the generic UI component, not the React module! labels May 11, 2023
@mui-bot
Copy link

mui-bot commented May 11, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8961--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 687.1 1,184.4 687.1 899.28 182.248
Sort 100k rows ms 698.5 1,302 1,182.6 1,069.8 203.487
Select 100k rows ms 246.4 394.1 286.2 296.16 51.133
Deselect 100k rows ms 172.5 371.6 217.3 252.6 82.157

Generated by 🚫 dangerJS against 8695c6a

cherniavskii added a commit to cherniavskii/data-grid-testing that referenced this pull request May 11, 2023
Comment on lines +114 to +119
React.useEffect(() => {
// For JSDOM test environment that doesn't support ResizeObserver
if (typeof ResizeObserver === 'undefined' && process.env.NODE_ENV === 'test') {
apiRef.current.computeSizeAndPublishResizeEvent();
}
}, [apiRef]);
Copy link
Member

@m4theushw m4theushw May 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have this code in

useEnhancedEffect(() => {
apiRef.current.computeSizeAndPublishResizeEvent();

Investigating more the bug I found the root problem. First, computeSizeAndPublishResizeEvent depends on the value of rootElementRef. By the time the effect above runs, in a test environment, this ref has not been set yet so it cannot store the dimensions. Consequentially, nothing is rendered and the test fails.

rootElementRef is set in

apiRef.current.register('public', { rootElementRef: rootContainerRef });
which means that only in the 2nd render its value will be available. The effect that calls computeSizeAndPublishResizeEvent runs in between the 1st and 2nd render. Mocking ResizeObserver is a hacky way to call computeSizeAndPublishResizeEvent again, and hopefully have the ref available this time. I suspect we didn't face this problem in our tests because of StrictMode.

I have an alternative solution in #8963. My solution bases on using inside the effect only the refs created in GridBody. When the effect runs we know for sure that all refs are available, so I pass the main element as an argument of computeSizeAndPublishResizeEvent, nothing of registering the ref in the API, which requires a 2nd render to work correctly. I tested with your test repo and it worked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we didn't face this problem in our tests because of StrictMode.

Yes, I can confirm this.
We don't have a global ResizeObserver mock and strict mode is the only reason the tests pass without it since useLayoutEffect runs twice

@cherniavskii cherniavskii deleted the resize-observer-jsdom branch May 12, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants