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] disable animations in unit testing #16288

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

lauri865
Copy link
Contributor

@lauri865 lauri865 commented Jan 21, 2025

An experiment to remove transitions from unit tests in order to make tests more stable. Lots of tests breaking due to act errors, which more often than not are related to the Transition component of react-transition-group. Would be good to disable it globally, and get rid of unnecessary waitFor calls as a result to make testing faster as well as more stable.

I don't have more time to dig into this now, so will leave it at this, if anyone wants to explore it further. Some tests broke as a result.

@lauri865 lauri865 changed the title Draft: [tests] override react transition group Draft: [tests] disable animations in unit testing Jan 21, 2025
@mui-bot
Copy link

mui-bot commented Jan 21, 2025

Deploy preview: https://deploy-preview-16288--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 1e8be24

@LukasTy LukasTy added the test label Jan 22, 2025
@LukasTy LukasTy changed the title Draft: [tests] disable animations in unit testing [test] disable animations in unit testing Jan 22, 2025
@LukasTy LukasTy marked this pull request as draft January 22, 2025 07:03
@@ -1,3 +1,5 @@
import * as React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import * as React from 'react';

@LukasTy
Copy link
Member

LukasTy commented Jan 22, 2025

@lauri865 @mui/xgrid WDYT about the updated approach of https://reactcommunity.org/react-transition-group/testing/? 🤔
Some problematic tests would have to be adjusted. 🙈

@lauri865
Copy link
Contributor Author

@lauri865 @mui/xgrid WDYT about the updated approach of https://reactcommunity.org/react-transition-group/testing/? 🤔 Some problematic tests would have to be adjusted. 🙈

Ah, nice find! Seems to break less tests it seems as well.

Wonder why they suggested an alternative approach here? 🤔
https://testing-library.com/docs/example-react-transition-group/

@LukasTy
Copy link
Member

LukasTy commented Jan 23, 2025

Ah, nice find! Seems to break less tests it seems as well.

Wonder why they suggested an alternative approach here? 🤔 https://testing-library.com/docs/example-react-transition-group/

I am not sure; maybe they are suggesting going for maximum efficiency and completely mocking the module to avoid any potential overhead.
I've used sinon.stub in the latest implementation to assert this idea. 😉
There are some Data Grid browser tests failing after this. 🤔

@lauri865
Copy link
Contributor Author

lauri865 commented Jan 23, 2025

Ah, nice find! Seems to break less tests it seems as well.
Wonder why they suggested an alternative approach here? 🤔 https://testing-library.com/docs/example-react-transition-group/

I am not sure; maybe they are suggesting going for maximum efficiency and completely mocking the module to avoid any potential overhead. I've used sinon.stub in the latest implementation to assert this idea. 😉 There are some Data Grid browser tests failing after this. 🤔

I'm not insinuating it's better. I found it difficult to fake the components due to some state being dependent on transition life-cycle events, hence I overrode props instead.

Are you use the latest one works? Transition component children are functions in many instances, and this should error out ("functions are not valid children" yaddayadda).

function FakeTransition({ children }) { return <React.Fragment>{children}</React.Fragment>; };

If the config prop works, I think it's probably the best bet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants