Adds toHaveErrorMessage assertion

[jacobparis]

What:

I implemented a basic toHaveErrorMessage assertion. It currently does not check for visibility (suggested by spec)

Original issue: #256

I'd like some recommendations for how to go about adding those two missing features. The logic to tell whether aria-invalid is set is already present in the toBeInvalid assertion and isVisible is in a similar boat.

My first reaction would be to reuse that functionality here, but as of right now there is no precedent in the codebase for one assertion to call the code of another.

I could re-implement it in this handler, but then there's two branches to maintain in parallel for the same functionality.

Checklist:

• Documentation
• Tests
• Updated Type Definitions
• Ready to be merged
  

[codecov]

Codecov Report

Merging #258 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #258   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        24    +1     
  Lines          315       323    +8     
  Branches        71        76    +5     
=========================================
+ Hits           315       323    +8     

Impacted Files	Coverage Δ
src/to-have-errormessage.js	100.00% <100.00%> (ø)
src/to-be-invalid.js	100.00% <0.00%> (ø)
src/to-be-disabled.js	100.00% <0.00%> (ø)

---

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 5e39222...069b5b3. Read the comment docs.

[gnapse]

I don't think what we need to check here is exactly all that isValid/isInvalid already check. The spec mentions specifically that aria-invalid is the key, and those other matchers cover other type of validity checking as well. The check needed here is simpler, merely check that the same element where aria-errormessage attribute is found must also have the aria-invalid="true" attribute present and set to true. That's as simple as what this function already does:

jest-dom/src/to-be-invalid.js

Lines 6 to 11 in 159a7a2

	function isElementHavingAriaInvalid(element) {
	return (
	element.hasAttribute('aria-invalid') &&
	element.getAttribute('aria-invalid') !== 'false'
	)
	}

I'm hesitant to extract such a simple functionality to a helper in utils, so feel free to copy the equivalent condition in that code to the file implementing this new assertion.

As for the other part, I also do not think that toBeVisible covers the visibility check we need here. It tests for actual "seeing it with your eyes" visibility, whereas with this one I suspect that screen-reader visibility is enough (e.g. making sure the element or one of its ancestors in the DOM tree does not have display: none and maybe not having also visibility: hidden).

However as we agreed on the discussion in the issue where you proposed this, I'd be ok if you omit that part, and we can add it when it becomes a problem. I'm not sure how to fully interpret the spec on this regard correctly. And also given that visibility is a tricky issue as it may be affected by the presence of stylesheets loaded in the document, something that usually in the context of jest tests of isolated components does not happen. We already have our fair share of issue reports of people complaining that matchers such as toBeVisible pass when they shouldn't, and it is for instance in cases where their components apply a class="hidden" to elements, but if that css class definition is not loaded and attached to the document, it does not indicate the element is hidden from jest-dom's POV.

[jacobparis]

Thank you for the detailed response. I think that clarifies all my concerns

[jacobparis]

I'm pretty happy with how this is looking right now. I could use some feedback on the test failure messages, and once we're all good with that I can move on to the documentation and types

[gnapse]

@jacobparis sorry for having dropped the ball on this one for so many days.

Those error messages look good to me. I updated this branch with the latest from master. Let's see how the CI behaves this time, and if it all passes I'll do a final review and approve and merge if everything's ok.

[php-coder]

@jacobparis Hi, could you, please, rebase this PR again to give @gnapse a chance to review and merge it?

[SevenOutman]

Any updates on this PR? I believe it's a better way to assert form validation rather than toHaveDescription.

cc @kentcdodds .

[gnapse]

The CI checks have failures. There's an issue with the test coverage. I believe you have to add tests to cover for some lines that are not being tested. This often is related to there being no tests for the negative assertions. For instance, this often happens when you test that both toHaveErrorMessage and not.toHaveErrorMessage are working as expected, but you also have to add tests for when toHaveErrorMessage throws an error because indeed there was no error message, AND you also need to test that not.toHaveErrorMessage throws when there is indeed an error message present.

It may help that you run npm run test:update which will generate locally a coverage report in the coverage/ folder. Open coverage/index.html and you'll be able to see what the uncovered lines of code are.

[SevenOutman]

Friendly ping. @jacobparis

[SevenOutman]

Friendly ping. @jacobparis

[gnapse]

Superseded by #370.