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.

Sign up for GitHub

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

Jump to bottom

add toMatchElement matcher #95

Merged
merged 2 commits into from
Jun 21, 2017
Merged

add toMatchElement matcher #95

merged 2 commits into from
Jun 21, 2017

Conversation

finnigantime
Copy link
Contributor

Implements .toMatchElement(), which addresses #37.

Example usage:

function FooComponent = () => {
  return (
    <div>
      <span id="foo" className="unexpectedClassName" />
    </div>
  );
};
expect(<FooComponent />).toMatchElement(
  <div>
    <span id="foo" className="bar" />
  </div>
);

Example failure message:

actual:
<div>
<span id="foo" className="unexpectedClassName" />
</div>
expected:
<div>
<span id="foo" className="bar" />
</div>

Notes:

  • I only am adding support for shallow() here. I think support for mount() could be added in the future.
  • The spacing is incorrect when calling debug() to print the shallow wrapper for expected and actual values. This is fixed in enzyme in Fix #925 - indentation is broken for debug() on shallow wrapper enzyme#926, so updating the enzyme dependency will fix this issue.
  • The comparison here calls debug() on both the actual and expected wrappers and then does an equality check on the resulting strings. Since debug() includes props and prop values, these are included in the comparison, unlike enzyme's .matchesElement(), which ignores props. In the future, it would be nice to have an option to call debug() without including props and prop values, so they could be ignored for the purposes of the comparison here.
  • It might also be nice in the future to add a variant of .toMatchElement() that takes a string as an arg, so you don't have to pull in all the dependent component modules to check how something renders.

@blainekasten
Copy link
Collaborator

Few thoughts here:

  1. We should support mount right away. Or have it give a useful error message that it's not supported. But that seems like just as much work as supporting mount.
  2. Is it worth waiting for an official release to merge this one then?
  3. I feel a bit uneasy about using debug internally anyhow. That API really isn't meant to be used like this. I'm nervous it could break at any time and be unexpected.

@blainekasten
Copy link
Collaborator

Also, 3.2.0 is out with most of your fixes!
https://github.com/blainekasten/enzyme-matchers/releases/tag/v3.2.0

@finnigantime
Copy link
Contributor Author

Nice, thanks!

Okay, I can look into making this work for mount. For 2, looks like you already cut the release, so it's okay - yeah, this is not related to the other fixes, but is just adding new API. For 3, I understand your concern. It also makes me nervous to depend on implementation detail of Enzyme's debug(). But, I think it's what we want here to stringify the wrapper tree, do a diff, and print the strings in case there is a diff. Rather than reimplement this, I want to use debug(). But I think it makes sense to add options flags to debug, which will allow debug() to add functionality while consumers that need it for specific purposes can control that behavior. For example, debug() currently includes prop vals, but for this comparison we probably don't want those, so we should add an includeProps flag to debug(). I wanted to make this change first before updating enzyme to add options to debug(), but we could try to update enzyme first. Let me know if this makes sense or if you prefer a different tack here.

@finnigantime finnigantime force-pushed the finnigantime/addToMatchElement branch from 3e2ad19 to ec932d9 Compare May 31, 2017 17:30
@finnigantime
Copy link
Contributor Author

@blainekasten updated to work with mount, and rebased against latest master.

@finnigantime
Copy link
Contributor Author

See enzymejs/enzyme#961, an attempt to allow ignoring props in the debug() output in enzyme. This also adds options to enzyme's debug(), which should enable a backwards-compatible codepath for enzyme-matchers to depend on this API even if the implementation/output of debug() changes for the default behavior when no options are specified.

@finnigantime finnigantime mentioned this pull request May 31, 2017
Closed
@blainekasten
Copy link
Collaborator

Sweet. This is cool man. Sorry for the delay. Just got back from a long vacation! Can you rebase and get the build passing? Then trim your commits down to a single commit? Thanks!

@finnigantime finnigantime force-pushed the finnigantime/addToMatchElement branch from ec932d9 to 3a6392f Compare June 19, 2017 17:34
@finnigantime
Copy link
Contributor Author

@blainekasten Welcome back! Sounds good - squashed and rebased. Let's see if the build passes.

@finnigantime
Copy link
Contributor Author

@blainekasten looks like there's an issue reporting status from CI

@finnigantime finnigantime force-pushed the finnigantime/addToMatchElement branch from 3a6392f to d00360b Compare June 21, 2017 15:17
@blainekasten
Copy link
Collaborator

@finnigantime I fixed it. So hopefully it'll work now

@blainekasten blainekasten merged commit d9128fa into enzymejs:master Jun 21, 2017
@blainekasten
Copy link
Collaborator

Thanks so much!

@finnigantime
Copy link
Contributor Author

Awesome, thank you! 👍

@blainekasten blainekasten mentioned this pull request Jun 21, 2017
Closed
@finnigantime finnigantime mentioned this pull request Jan 24, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@finnigantime @blainekasten