Add toHaveAttribute custom matcher (closes #43) #44

gnapse · 2018-04-05T13:48:22Z

Feedback is welcome on the code to build the message for the custom matcher. It got a bit tricky but I think it's worth it to give human-friendly messages of what went wrong, and there are quite a few cases to cover there.

Closes #43

antsmartian · 2018-04-05T14:32:34Z

src/jest-extensions.js

+  printReceived,
+  printExpected,
+  stringify,
+  RECEIVED_COLOR as receivedColor,


Just wondering here, can't we use printReceived? Is that RECEIVED_COLOR does something apart from printReceived?

Yes. printReceived uses RECEIVED_COLOR internally, but it also stringifies the argument. But in the new code I added I wanted to show attribute-name="value" all in the received or expected color, without stringifying all of it. If I used printReceived/printExpected I'd have the whole attribute-name="value" itself surrounded by double quotes (and possibly those inner quotes escaped then).

antsmartian · 2018-04-05T14:34:46Z

src/jest-extensions.js

+          expectedValue,
+          receivedValue,
+        )
+        const matcher = matcherHint(


We have used assertMessage method to handle the messages. Can't we re-use them? Like we have done it over here: https://github.com/gnapse/react-testing-library/blob/bfeb26aa29e2b36cbf254a6cb8dc6934e32a055e/src/jest-extensions.js#L89

@antoaravinth Not in this case. The messages I wanted to generate do not comply with the predefined format that assertMessage uses.

BTW, not even the two already existing custom matchers used this function. It's only being used in one of them. I do not think we can expect all or even most matchers to always adhere to this expected/received constrained message format. I was checking at jest's own codebase and how they approach generating messages, and the most common functionality they have across all them is encoded in all the functions they also publish in jest-matcher-utils and that we're using here.

Yes, I got it. Just wanted to check. Cool, thanks!

That being said, although I do not object myself to how the code ended up in this function for generating the message, I get it if it looks too complex, and I'm of course open to any suggestions to improve it or even the resulting messages as well. I would not like though to do so by sacrificing too much of the friendliness of the resulting messages.

This matcher, although simple in the actual condition being tested, covers a few cases (e.g. it can test if the attribute is merely present or not, or it can test also that it has a specific value). Therefore the messages for the positive and negative cases, each of these handling the two cases of the attr value being tested or not, yields four different kind of resulting messages.

antsmartian · 2018-04-05T14:36:04Z

@gnapse This is great! Left few comments!

kentcdodds

Looking good! Could you take a screenshot of what a failure looks like for both the .toHaveAttribute and .not.toHaveAttribute cases?

gnapse · 2018-04-05T18:34:44Z

Sure, here it is, the four different possible cases:

// Expect attribute to be present and with a certain value
.toHaveAttribute(attr, value)
.not.toHaveAttribute(attr, value)

// Expect attribute to be present, regardless of its value
.toHaveAttribute(attr)
.not.toHaveAttribute(attr)

(The lines with the bullet points in front of them are the labels given to the it() test cases that I created to generate this screenshot. These are not part of the messages output of this matcher.)

kentcdodds · 2018-04-05T19:13:10Z

Awesome. Could you update it so it resembles the errors from other assertions?

Like this:

Specific differences are:

There's a chalk.dim comment after the assertion that could be handy: // element.getAttribute('aria-expanded') === 'true'. I'm fine to add chalk as a dependency for this 👍 Most people will already have it installed anyway.
Add a colon after the "Expected the element..." and the "Instead:"
Add a newline and two spaces before the expected and actual

Let me know if you have questions :)

gnapse · 2018-04-05T20:06:50Z

@kentcdodds I did points 2 and 3 above.

However, regarding the first point, that is apparently a relatively new feature of the jest helper matcherHint, passing to it a comment: '...' in the last argument that's for options. So in theory I'd not even need to add a direct dependency to chalk, because matcherHint generates this internally.

However, it was odd that I did not get that comment when using .toBe in my own app's tests. That's when I realized I'm not on the latest jest version, and though I can upgrade, you can't guarantee that all the users of this lib will be either.

Do you prefer if I add the comment myself using chalk directly? Or would it be better to add it in this comment option, and it'll appear to users or not, depending on the version of jest that they're using (I'd go for the latter option if you ask me).

kentcdodds · 2018-04-05T20:09:32Z

Let's use the comment option. That's great to know! I wasn't aware of that feature :)

gnapse · 2018-04-05T20:14:56Z

Me neither! I learned it while working on this. That's a huge part of what I love about contributing to open source.

kentcdodds · 2018-04-05T20:39:48Z

src/jest-extensions.js

+    ? `element.hasAttribute(${stringify(name)})`
+    : `element.getAttribute(${stringify(name)}) === ${stringify(value)}`
+}
+
 const extensions = {
  toBeInTheDOM(received) {
    getDisplayName(received)


I just noticed that this actually isn't doing anything. I'm pretty sure we should assign it a value called displayName and use that somehow or remove it entirely... Probably good for another PR though.

Yes, I noticed that too, but I preferred no to touch it here as it's unrelated.

I do have a few code cleanup proposals for this file, that I was meaning to provide in a separate PR. For instance, the pattern of returning something different on the matchers depending on wether they passed or not seems redundant to me.

Also it'd be good to bring the other two matchers messages up to the standards that you have proposed for .toHaveAttribute.

Yes, I'd love it if you could clean this file up a bit in another PR! Let's just see what we need to do to increase coverage back to 100%.

Open coverage/lcov-report/index.html in your browser to see what's left 👍

codecov · 2018-04-05T21:06:44Z

Codecov Report

Merging #44 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #44   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines          85     98   +13     
  Branches       19     22    +3     
=====================================
+ Hits           85     98   +13

Impacted Files	Coverage Δ
src/jest-extensions.js	`100% <100%> (ø)`	⬆️
src/extend-expect.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34c974d...c191008. Read the comment docs.

kentcdodds

Most excellent!

…elease-15.10.6 Update semantic-release to the latest version 🚀

gnapse added 2 commits April 5, 2018 10:42

Add toHaveAttribute custom matcher

0618715

Add gnapse as contributor

bfeb26a

antsmartian reviewed Apr 5, 2018

View reviewed changes

antsmartian requested a review from kentcdodds April 5, 2018 14:35

kentcdodds reviewed Apr 5, 2018

View reviewed changes

Generate messages more consistent with jest's other matchers

12db312

Add comment to matcher hint

aaf03ae

kentcdodds reviewed Apr 5, 2018

View reviewed changes

Add tests to cover toHaveAttribute custom matcher

c191008

kentcdodds approved these changes Apr 5, 2018

View reviewed changes

kentcdodds merged commit 7267acd into testing-library:master Apr 5, 2018

gnapse deleted the pr/to-have-attribute branch April 5, 2018 21:08

antsmartian mentioned this pull request Apr 6, 2018

CleanUp for toBeInTheDom and toHaveTextContent #45

Closed

This was referenced Apr 6, 2018

Refactor custom matchers (closes #45) #46

Closed

Refactor custom matchers testing-library/dom-testing-library#1

Merged

rphumulock mentioned this pull request Aug 8, 2019

Lacking of documentation on "toHaveAttribute" testing-library/testing-library-docs#210

Closed

lucbpz pushed a commit to lucbpz/react-testing-library that referenced this pull request Jul 26, 2020

Merge pull request testing-library#44 from Gpx/greenkeeper/semantic-r…

c110df7

…elease-15.10.6 Update semantic-release to the latest version 🚀

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add toHaveAttribute custom matcher (closes #43) #44

Add toHaveAttribute custom matcher (closes #43) #44

gnapse commented Apr 5, 2018 •

edited

Loading

antsmartian Apr 5, 2018

gnapse Apr 5, 2018

antsmartian Apr 5, 2018 •

edited

Loading

gnapse Apr 5, 2018

antsmartian Apr 5, 2018

gnapse Apr 5, 2018

antsmartian commented Apr 5, 2018 •

edited

Loading

kentcdodds left a comment

gnapse commented Apr 5, 2018 •

edited

Loading

kentcdodds commented Apr 5, 2018

gnapse commented Apr 5, 2018 •

edited

Loading

kentcdodds commented Apr 5, 2018

gnapse commented Apr 5, 2018

kentcdodds Apr 5, 2018

gnapse Apr 5, 2018 •

edited

Loading

kentcdodds Apr 5, 2018

codecov bot commented Apr 5, 2018

kentcdodds left a comment

Add toHaveAttribute custom matcher (closes #43) #44

Add toHaveAttribute custom matcher (closes #43) #44

Conversation

gnapse commented Apr 5, 2018 • edited Loading

antsmartian Apr 5, 2018

Choose a reason for hiding this comment

gnapse Apr 5, 2018

Choose a reason for hiding this comment

antsmartian Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

gnapse Apr 5, 2018

Choose a reason for hiding this comment

antsmartian Apr 5, 2018

Choose a reason for hiding this comment

gnapse Apr 5, 2018

Choose a reason for hiding this comment

antsmartian commented Apr 5, 2018 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

gnapse commented Apr 5, 2018 • edited Loading

kentcdodds commented Apr 5, 2018

gnapse commented Apr 5, 2018 • edited Loading

kentcdodds commented Apr 5, 2018

gnapse commented Apr 5, 2018

kentcdodds Apr 5, 2018

Choose a reason for hiding this comment

gnapse Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

kentcdodds Apr 5, 2018

Choose a reason for hiding this comment

codecov bot commented Apr 5, 2018

Codecov Report

kentcdodds left a comment

Choose a reason for hiding this comment

gnapse commented Apr 5, 2018 •

edited

Loading

antsmartian Apr 5, 2018 •

edited

Loading

antsmartian commented Apr 5, 2018 •

edited

Loading

gnapse commented Apr 5, 2018 •

edited

Loading

gnapse commented Apr 5, 2018 •

edited

Loading

gnapse Apr 5, 2018 •

edited

Loading