-
Notifications
You must be signed in to change notification settings - Fork 46.2k
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
Support nonRenderPhase updates in ShallowRenderer #14841
Support nonRenderPhase updates in ShallowRenderer #14841
Conversation
Details of bundled changes.Comparing: 0e4135e...705f5e1 react-test-renderer
Generated by 🚫 dangerJS |
// A sanity check to ensure two maps do not get used together | ||
invariant( | ||
this._didScheduleRenderPhaseUpdate === false, | ||
'non-render phase updates should never occur in combination with render phase updates', |
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.
Why would they not?
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 couldn't think of a case where that would happen, at least in the case they do this invariant would hit. If you can think of a test for it I can enable it and add the logic for that code path :)
} | ||
updatesMap = this._renderPhaseUpdates; | ||
} else if (componentIdentity === this._previousComponentIdentity) { | ||
// This means an update has happened after the function component has |
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.
Should it also rerender in this case?
It would be inline with class component behavior, covering tests like
rendered.props.onClick();
expect(shallowRenderer.getRenderOutput()).toEqual(... updated ...);
which is what I think a lib like enzyme would do internally
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'm not sure how Enzyme works, but in this case, it's triggering an update to the update queue. There is no re-render, which seems more in line with how shallow renderer works (you explicitly call render
).
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.
But don't shallow-render Updater
do this already? (immediately change the state and rerender)
react/packages/react-test-renderer/src/ReactShallowRenderer.js
Lines 154 to 159 in 0e4135e
this._renderer._newState = { | |
...currentState, | |
...partialState, | |
}; | |
this._renderer.render(this._renderer._element, this._renderer._context); |
(i'm not sure who calls enqueueSetState, so I might be missing something)
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.
Yeah setState should trigger a render.
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.
Ah, my mistake. I've changed the PR to do a re-render.
useState seems to be working with this but useEffect don't, is this intended? |
AFAIK even old React lifecycle methods like In general I wouldn't recommend using shallow renderer for anything more involved than asserting on the result. If you want to test |
That approach feels weird to me... I need to know the internals of every sub component to be able to make assertions, and that becomes really messy as you go up in the tree. If a sub component changes, than my unit test has to change too... I don't know, maybe I'm missing something, but I've never seen an example showing how this approach scales. |
This is a long discussion and I'm not sure I'm ready to do it in this PR. :-)
You need to test what matters to the user. This approach is described here: https://testing-library.com/docs/react-testing-library/intro. The user doesn’t care how you split your app into components. If you refactor a Of course you’re right that you don’t usually want a single test to get too deep. That’s where mocking helps. In Jest, you can do jest.mock('../MyButton', () => 'button') or jest.mock('../SomeContent', () => (props) => '(My content)') and that would shim the rendered component with a fake version. That allows you to pick where your test “ends“. Other test runners may or may not provide similar capabilities. In either case, this is a long discussion that’s irrelevant to this PR — so let’s have it elsewhere as long as we have a clear question and goals in mind. I’d rather this not turn into a giant argument about testing because people tend to be very opinionated about that. |
I get what your saying, I thought about that a lot and eventually got to this mocking approach, but I always feel I'm doing wrong, you know? I guess I'm too obsessed with unit testing, maybe I need to change how I think about tests. Anyway, you are right, this is not the place for this and thank you very much for answering! |
Any news on when this will be merged ? |
Fixed in #15120 |
Addresses the issue with #14840.
Following up from the fix in #14802 – which although somewhat correct, it didn't make sense when aligned with the comments. I've addressed the comments and approach so it should fix the original issue whilst keeping non-render phase updates and render-phase updates separate.