properly handling invalid scryRenderedDOMComponentsWithClass args #6529
Conversation
properly handling invalid scryRenderedDOMComponentsWithClass args properly handle invalid scryRenderedDOMComponentsWithClass args
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| @@ -178,9 +178,6 @@ var ReactTestUtils = { | |||
| * @return {array} an array of all the matches. | |||
| */ | |||
| scryRenderedDOMComponentsWithClass: function(root, classNames) { | |||
| if (!Array.isArray(classNames)) { | |||
There was a problem hiding this comment.
probably
+ invariant(
+ classNames !== undefined,
+ 'TestUtils.scryRenderedDOMComponentsWithClass expects a ' +
+ 'className as a second argument.'
+ );
if (!Array.isArray(classNames)) {There was a problem hiding this comment.
@sarbbottam I also thought about doing it that way. The reason that I did it the way I did was to make sure the errors were as useful and specific as possible. For example. If we were to do it the way that you're recommending, if someone makes a mistake and entered the className foo as the only argument. They will no longer receive the helpful error that states that foo is not an instance of a component but will be presented with this new error instead Invariant Violation: TestUtils.scryRenderedDOMComponentsWithClass expects a className a second argument , which, to me is more ambiguous and unclear of what part of their test they need to fix. I think the second (new) error is more helpful when it is only presented to the user after we have verified that the first argument is a valid instance of a component. That's my reasoning but I'm opening to updating the code to your implementation if that's the consensus. Thanks for the feedback!
There was a problem hiding this comment.
Thanks @ipeters90 for the detailed explanation.
if someone makes a mistake and entered the className foo as the only argument
I guess then there should have been a check for root as well.
There was a problem hiding this comment.
Inside of scryRenderedDOMComponentsWithClass, which takes in the root (instance of component) as the first argument, it calls ReactTestUtils.findAllInRenderedTree(root) which checks if root is a valid instance of a react component.
Reference: https://github.com/facebook/react/blob/master/src/test/ReactTestUtils.js#L168-171
So if someone calls scryRenderedDOMComponentsWithClass with no arguments or with an invalid root argument, it will throw an appropriate error (Invariant Violation: findAllInRenderedTree(...): instance must be a composite component). That seems sufficient to me. Do you think there's additional checks needed ? @sarbbottam
) properly handling invalid scryRenderedDOMComponentsWithClass args properly handle invalid scryRenderedDOMComponentsWithClass args (cherry picked from commit 9df54e0)
Fixes #6493
If one argument is passed in to
scryRenderedDOMComponentsWithClassand it's not a valid instance of a react component, you will still get the standardInvariant Violation: findAllInRenderedTree(...): instance must be a composite componenterror message as usual.If a valid instance of a component is passed in as the first argument but a
classNameis not passed in as a second argument, the user will be presented with this error message:Invariant Violation: TestUtils.scryRenderedDOMComponentsWithClass expects a className a second argument.