Refactor custom matchers (closes #45)

[gnapse]

This is a proposed refactor to the custom matchers code, as briefly discussed in #44 (comment):

• Use common message format across all custom matchers
• Make it easy to adopt the same format in any future matchers
• Simplify code, remove unneeded if/else logic

Closes #45

Proposed changes not yet performed (pending discussion)

• Make each custom matcher a named export, instead of bundling them in an object as default export
• Better organize in a sub folder with one file per matchers and some utils file for common stuff

Pending to be fixed

• Code coverage is not full because getDisplayName never gets called during the tests with a class as argument, only with html elements. I wonder if that is expected, given that this library is only dealing with rendered html and not the virtual dom, or is it?

[antsmartian]

Thanks for this!

Better organize in a sub folder with one file per matchers and some utils file for common stuff

There are only 3 custom matchers, with few common utils like getMessage, checkHtmlElement. Do we really move them per file basis? May be / May be not :)

Code coverage is not full because getDisplayName never gets called during the tests with a class as argument, only with html elements. I wonder if that is expected, given that this library is only dealing with rendered html and not the virtual dom, or is it?

What if someone does this:

expect(queryByText('notindom').toBeInTheDOM()

then we decided to throw exception like:

The given subject is a object, not an HTMLElement

And also I guess you had to add checkHtmlElement for toBeInTheDOM.

[gnapse]

Agree that it might not be a good idea to split the custom matchers. That's why I did not do it, just wanted to throw the idea out there. What do you think about exporting each of them individually as named exports? Not that's too important right now, but I find it odd to have them all bundled in an object, when they are independent pieces.

About this

And also I guess you had to add checkHtmlElement for toBeInTheDOM.

The whole point of .toBeInTheDOM is that it could be called with something that is not found in the DOM, so it won't be a HTML element. We cannot throw an exception in that case. The actual matcher will take care of that when we return pass: false.

Regarding the getDisplayName issue

I guess what I need to do is to add a test case where we expect something that's not null (but also not a html element) to be in the DOM. That has to be a test that does not use queryByText or any of the other query* functions, because those all return either a html element of null. I'll do that.

[gnapse]

Ok, sorry about the rushed response. I fully get what you mean now. I added the check for html elements in toBeInTheDOM, but only when the argument is not null. If the argument is null or undefined I leave it to fail normally, because it does not pass.

I was confused at first because this check was never there before, just a superfluous call to getDisplayName whose return value was not being used.

[codecov]

Codecov Report

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

@@          Coverage Diff          @@
##           master    #46   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines          98     92    -6     
  Branches       22     24    +2     
=====================================
- Hits           98     92    -6

Impacted Files	Coverage Δ
src/jest-extensions.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 7267acd...eaa0cb0. Read the comment docs.

[antsmartian]

Agree that it might not be a good idea to split the custom matchers. That's why I did not do it, just wanted to throw the idea out there. What do you think about exporting each of them individually as named exports? Not that's too important right now, but I find it odd to have them all bundled in an object, when they are independent pieces.

May be we can take this up. @kentcdodds Any thoughts?

The whole point of .toBeInTheDOM is that it could be called with something that is not found in the DOM, so it won't be a HTML element. We cannot throw an exception in that case. The actual matcher will take care of that when we return pass: false.

Yes, but since we have checks like:

checkHtmlElement(htmlElement)
const textContent = htmlElement.textContent // might be undefined if its not an HTMLElement

What if the htmlElement is not an HTMLInstance? We might end up in other issues. Yes, as you said we have to check error logic and pass the control to matchers, so that it ends up in fail test cases.

[kentcdodds]

Could we move this over to dom-testing-library?

Sorry to change things underneath you!

[gnapse]

No problem, it makes sense. I'll re-create the PR over there.