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

Add equals method to ShallowWrapper. #124

Merged

Conversation

themouette
Copy link
Contributor

Fixes #122

expect(wrapper.equals(b)).to.equal(true);
});

});
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add 2 tests to this?

say we have a composite componente <Foo /> which renders `...

expect(shallow(<Foo />).equals(<Bar />)).to.equal(true);
expect(shallow(<Foo />).equals(<Foo />)).to.equal(false);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@lelandrichardson
Copy link
Collaborator

This is great. Thanks for contributing!

One thing I'd like us to consider:

What if we had this be an extension of the .is(selector) API instead?

The issue in this case would be the fact that this would actually conflict with #110 unless we had some way of differentiating a JSX element from an object that just looks like a JSX element...

thoughts? cc @blainekasten @ljharb @hartzis

.equals() might be the way to go...

@lelandrichardson
Copy link
Collaborator

cc @vesln for how we might want to use this in chai-enzyme.

@themouette
Copy link
Contributor Author

.is(<Node />) is appealing, but it would require similar API in ReactWrapper, and I'm wondering how this would behave:

const Bar = () => <p />;
const Foo = () => <div><Bar /></div>;

// obviously this returns true:
render(<Foo />).is(<div><p /></div>);
// should this returns true as well?
render(<Foo />).is(<div><Bar /></div>);

@blainekasten
Copy link
Contributor

Thanks for the cc @lelandrichardson. I'm personally not that excited about this API in general. My thoughts are that this gives the wrong intention to testing components.

What I think should be the primary goal in testing react components, is that given a certain set of state/props/context, an expected HTML render is asserted. Not an expected JSX call. At that point, you're just duplicating code and making sure it doesn't change. But the purpose of tests is to assert results, not implementations. Because I don't care if code get's refactored in a different way as long as the end result is the same.

If I used this method, I could see somebody doing something like this:

const component = render(
  <div><b/></div>
);

component.equals(<div><b/></div>) // true

But then, say a refactor happens to this

Foo = <b/>
const component = render(
  <div><Foo/></div>
);

now all of a sudden, the result is the exact same, but my tests have failed.

So I just think this encourages the wrong habits.

@vesln
Copy link
Contributor

vesln commented Jan 19, 2016

@lelandrichardson it should be quite straight forward for chai-enzyme to implement.

expect(xyz).to.equal(xyz)

@hartzis
Copy link
Contributor

hartzis commented Jan 20, 2016

Great explanation @blainekasten.

Though I don't agree completely that the primary goal of testing react components needs to be html output. Using shallow render you're able to keep the scope of a test to a single component and make assertions about props being passed and what nodes/components will be 'rendered'.

I personally wouldn't test using this, but I could see it possibly being useful for shallow render testing. I also like @vesln's idea, but it does tie you to chai.

@lelandrichardson
Copy link
Collaborator

I agree with @hartzis . While I think this API could be used in a way that I wouldn't encourage (as you describe), I think the same could be said for most of the APIs on enzyme. This provides the ability for someone to describe what a specific sub-tree of the resulting output looks like, and I think that is powerful.

After giving it some thought, I think this is fine to be on .equals() instead of .is() as well.

As a result, this PR is ready to merge.

* @returns {Boolean}
*/
equals(node) {
return nodeEqual(this.node, node);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you wrap this inside a return this.single(() => nodeEqual(this.node, node));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@themouette
Copy link
Contributor Author

rebased onto master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor New stuff.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants