[test] react-test-renderer coverage #16523
Conversation
Details of bundle changes.Comparing: 783b693...ee6084a
|
There was a problem hiding this comment.
We need to find a stance on react-test-renderer support. This negatively impacts the behavior in the actual app. We do have a dependency on the DOM. We should rather encourage people to properly mock the refs with react-test-renderer. Any thoughts from the core team?
|
I agree, I don't think that we should go down this path either. An extra process.env.NODE_ENV === 'test' check at the custom effet level could be enough? |
|
@eps1lon What do you think of closing the related issue as won't fix? I have tried to add a new test in the function testJsonRenderer(element) {
it('should render without errors in ReactTestRenderer', () => {
ReactTestRenderer.create(element).toJSON();
});
}But it fails non-deterministaclly. Sometimes, the effect triggers. |
|
I could isolate the weird case to: import React from 'react';
import ReactTestRenderer from 'react-test-renderer';
import Slider from './Slider';
import SnackbarContent from '../SnackbarContent';
describe.only('<Slider />', () => {
it('what?', () => {
ReactTestRenderer.create(<Slider value={0} />);
ReactTestRenderer.create(<SnackbarContent message="conform" />);
// The slider's useEffect triggers and crash 🙃
});
});Maybe it's a bug in the React module (only a guess). |
|
Another possible solution: we could have defensive checks in the test environment for all the effects. @dondi I'm out of good options. I would propose we only encourage people to follow https://reactjs.org/docs/test-renderer.html#ideas. Does anyone have a potential solution? |
|
@oliviertassinari One thought: in my sandbox example from the original issue (https://codesandbox.io/s/create-react-app-jl67r) I showed that |
|
How do you envision it, how would it work? |
|
Scratch that, I just realized that a change at the level of Material UI’s test setup isn’t practical either, because the failure will occur in other people’s test fixtures. In the meantime, two other ideas came to mind. One works but it is isolated (i.e., it would have to be applied for every component that is discovered to have issues with null refs within tests), and I am still working on the other one, but if it works it would involve a broader change to the code base. Idea 1This change performs the “skip” on a const syncHeight = React.useCallback(() => {
if (process.env.NODE_ENV === 'test' && inputRef.current === null) {
return;
}Idea 2It appears that the general issue might actually be describe('ReactTestRenderer behavior', () => {
it('should render without errors in ReactTestRenderer', () => {
ReactTestRenderer.create(<Slide {...defaultProps} />);
});
});Only Slide, TextareaAutosize, and ModalManager invoke export const muiGetComputedStyle = typeof window !== 'undefined' && process.env.NODE_ENV !== 'test'
? window.getComputedStyle
: node => ({}) // Or whatever minimal style object is needed for dependent code to work.I’m in the middle of exploring this right now but I don’t know where to put it, and it might not be a good idea anyway so I may as well ask first 😅 I think what I’ll do for now is commit what I have for Idea 1, then wait for feedback on whether Idea 2 is worth a try. (without endangering the integrity of the code base) |
|
I think you need to wrap this in |
|
@eps1lon Oops sorry just missed your comment while committing and pushing. I’m unclear on wrapping in |
-- https://reactjs.org/docs/test-utils.html#act If I remember correctly 16.9 will also issue warnings if you mix those. |
|
Oh I see what you mean now! Great, I will try that. Thanks @eps1lon! |
|
@eps1lon Is this what you had in mind? I tried it without the describe('ReactTestRenderer behavior', () => {
it('should render without errors in ReactTestRenderer', () => {
ReactTestRenderer.act(() => {
ReactTestRenderer.create(<TextareaAutosize />);
});
});
}); |
|
Should've mentioned that I responded to @oliviertassinari #16523 (comment) |
|
@eps1lon Oh I see, it was in reference to that other case. Thanks for the clarification; I’ll back off from that then. |
|
@eps1lon Thanks, the |
oliviertassinari
force-pushed
the
textfield-multiline-in-jest
branch
from
9c7e849 to
c26a2c8
Compare
oliviertassinari
force-pushed
the
textfield-multiline-in-jest
branch
2 times, most recently
from
bb2d23e to
b22af3a
Compare
|
@eps1lon I have updated the pull request with a new generic test. What do you think of the approach? I can clean it up if we agree on this direction. |
|
I have looked at the Tabs failure. The issue comes from the fact that the mocked elements have no children. There is also a couple that comes from the nonsupported portal API. For the fails on the transition components, I don't know. It's suspicious. |
|
Might still be a nice way of documenting how dependent on the DOM some components are. You know, as a way of tracking progress towards RN 😉 |
|
Wow lots of activity here! Definitely going beyond my knowledge level of the codebase, but that’s to be expected 🙂 Since this PR came out of an issue I reported, I just wanted to put in some context for how I encountered the issue. This came about because I was writing a test suite for a project of my own that uses Material-UI. It was originally based on MUI v1.x and I finally had the time to port it to MUI v4.x. That was when I realized that a number of component tests started failing; upon triage, I realized that they were all components that (a) were using snapshots via I'm bringing this up because, based on what I'm seeing, it looks like this PR is evolving into a universal test layer that checks how MUI components behave with Depending on the answer to this question, it looks like there’s a possible range of follow-ups needed, ranging from just documenting this change so that affected devs can update their test suites, or if the intent is to avoid having everyone revise their suites, then it looks like additional work will be needed after this test layer is done so that the components themselves will not break Hope this is clear…I know this is now over my head so if I’m missing anything just let me know. But I figured I’d be momentarily selfish and check on how this might affect the project for which I discovered this issue in the first place 🙂 Thanks very much for your time and attention! |
|
@dondi I believe that @eps1lon and I tend to lean toward the documentation approach. The documentation could be the related issue itself. There is not that many feedback on the problem. We could have people search the issues, it could be a good solution enough and later graduate in a documentation page if it drain enough traffic. For the portal problem, a workaround is exposed in facebook/react#11565 (comment). |
|
OK, thank you for the clarification! Documentation via issues works for me—that’s how I discovered the I don’t think I can be very helpful with the transition failure, but since I have the branch anyway, I can take a peek when I get the chance. Thank you again! |
|
We could also look into the shallow support https://reactjs.org/docs/shallow-renderer.html |
oliviertassinari
changed the title
oliviertassinari
force-pushed
the
textfield-multiline-in-jest
branch
from
b22af3a to
52888ca
Compare
|
@eps1lon I have dived into the fails. I have fixed the tab ones. The portal ones are expected. The react-transition-group fails come from an issue with .findDOMNode() + appear={true}. |
oliviertassinari
force-pushed
the
textfield-multiline-in-jest
branch
from
52888ca to
924c1e1
Compare
oliviertassinari
force-pushed
the
textfield-multiline-in-jest
branch
from
924c1e1 to
ee6084a
Compare
oliviertassinari
added
PR: good for merge
and removed
component: TextareaAutosize
|
Great to see this! Thanks very much @oliviertassinari and @eps1lon for your time, feedback, and work on this issue. |
I sort of just inferred the
describeanditlabels for the tests that would fail if the fix (i.e., thenullcheck andreturninsyncHeight) is not present. I’ll be happy to revise these to whatever matches codebase conventions better based on your feedback. Thanks very much!Closes #16491