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

Changed tests to highlight a bug with contains, fixed the bug in Utils/containsChildrenSubArray #226

Merged
merged 1 commit into from
Mar 7, 2016

Conversation

CurtisHumphrey
Copy link
Contributor

moved checker to another line to make linter happy

<div>Goodbye</div>,
<span>More</span>,
Copy link
Member

Choose a reason for hiding this comment

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

Could you include both tests (with both orderings, i mean) to ensure we don't regress in the future?

@CurtisHumphrey
Copy link
Contributor Author

Sure I added both cases as passes1 and passes2 @ljharb

@ljharb
Copy link
Member

ljharb commented Mar 3, 2016

Thanks! hmm - i can't find anything in the documentation that talks about whether order should or shouldn't matter if you pass an array of nodes. However, the usage of some means that shallow(<A />).contains([<A />, <B />]) would be expected to pass (which could use a note in the docs) - so I'd expect shallow(<A />).contains([<B />, <A />]) to pass as well.

@CurtisHumphrey it'd be awesome if you could rebase this on top of latest master - if not, no worries.

@lelandrichardson what do you think about updating the docs to explicitly mention the "any" semantics as well as that ordering doesn't matter?

@CurtisHumphrey
Copy link
Contributor Author

@ljharb I just rebased

@lelandrichardson
Copy link
Collaborator

@ljharb I think you may have misread this function. If the render tree contains the entire array being passed in, then it matches. This means that order does matter. The bug was that this was only working properly if the array in the tree started with this sub-array.

Thanks for catching this, @CurtisHumphrey !

@lelandrichardson
Copy link
Collaborator

@CurtisHumphrey do you mind rebasing and squashing, and I'll merge this in? Thanks!

@ljharb
Copy link
Member

ljharb commented Mar 5, 2016

@lelandrichardson ah, thanks for clarifying. I definitely misread the test, but looking at it again, what you say is indeed represented in the tests :-)

…renSubArray

moved checker to another line to make linter happy
Updated based on feedback from PR
@CurtisHumphrey
Copy link
Contributor Author

@lelandrichardson rebased and squashed

lelandrichardson added a commit that referenced this pull request Mar 7, 2016
[contains] Fix bug for matching sub arrays
@lelandrichardson lelandrichardson merged commit 43f752d into enzymejs:master Mar 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants