-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
properly handling invalid scryRenderedDOMComponentsWithClass args #6529
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
probably
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.
@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 thatfoo
is not an instance of a component but will be presented with this new error insteadInvariant 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ipeters90 for the detailed explanation.
I guess then there should have been a check for
root
as well.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.
Inside of
scryRenderedDOMComponentsWithClass
, which takes in theroot
(instance of component) as the first argument, it callsReactTestUtils.findAllInRenderedTree(root)
which checks ifroot
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 invalidroot
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 ? @sarbbottamThere 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.
Got it, thanks!!