fix: normalize expected value in toContainHTML #349

just-boris · 2021-03-18T23:06:11Z

What:

resolve #347

Why:

Because this is worth fixing

How:

Added getNormalizedHtml function, that passes the content to the HTML parser.

This caused two changes in the test case with invalid HTML: <span data-testid="child"></div> becomes <span data-testid="child"></span>

Checklist:

Documentation n/a
Tests
Updated Type Definitions n/a
Ready to be merged

just-boris · 2021-03-18T23:07:56Z

src/__tests__/to-contain-html.js

@@ -18,21 +18,23 @@ describe('.toContainHTML', () => {
    const nonExistantElement = queryByTestId('not-exists')
    const fakeElement = {thisIsNot: 'an html element'}
    const stringChildElement = '<span data-testid="child"></span>'
-    const incorrectStringHtml = '<span data-testid="child"></div>'
+    const stringChildElementSelfClosing = '<span data-testid="child" />'
+    // const incorrectStringHtml = '<span data-testid="child"></div>'


The new change also normalizes this invalid HTML, so this also now passes:

expect(child).toContainHTML('<span data-testid="child"></div>'); // normalized to `<span data-testid="child"></span>`

codecov · 2021-03-18T23:08:09Z

Codecov Report

Merging #349 (c0e8a1b) into main (84fe8e0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #349   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines          539       545    +6     
  Branches       199       200    +1     
=========================================
+ Hits           539       545    +6

Impacted Files	Coverage Δ
src/to-contain-html.js	`100.00% <100.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 84fe8e0...c0e8a1b. Read the comment docs.

just-boris · 2021-03-18T23:12:18Z

src/to-contain-html.js

+  if (typeof htmlText !== 'string') {
+    throw new Error(`.toContainHTML() expects a string value, got ${htmlText}`)
+  }


Without this change, the nonExistantElement tests started failing. This value is basically an equivalent of expect(element).toContainHTML(null) which turned into a string.includes(null), producing false.

Now the htmlText always gets converted to a string, so this extra check is needed to keep the assertions failing where they were before.

just-boris · 2021-03-18T23:14:12Z

Opened this as a draft, as there are two maybe-major changes, see the comments above.

I am open for a discussion how to proceed with that

gnapse · 2021-03-19T17:58:59Z

@just-boris Thanks for this! Can you take this PR to a valid non-draft state (e.g. uncomment or remove the commented code) so that I can review. I cannot get a full idea of what's going on or what you are proposing without knowing what about all that commented out code.

just-boris · 2021-03-20T00:46:48Z

@gnapse
Updated the PR. It introduces two notable changes besides fixing the #347

non-string values throw a different error now. It was an assertion error and now this is precise .toContainHTML() expects a string value, got ${htmlText}
Invalid HTML gets normalised now, passing the assertion if the result is the same as the actual content

gnapse

I'm mostly OK with these changes. I left a comment on the only thing that worries me. Would like to hear what you think before merging this.

gnapse · 2021-03-23T13:25:21Z

src/__tests__/to-contain-html.js

-    expect(grandparent).not.toContainHTML(incorrectStringHtml)
-    expect(parent).not.toContainHTML(incorrectStringHtml)
-    expect(child).not.toContainHTML(incorrectStringHtml)
-    expect(child).not.toContainHTML(incorrectStringHtml)


Do we need to remove the test on the invalid html? What happens after these changes if we give invalid html like the one in this string? It'd be nice to know and add a test for that. And hopefully that what it does is still close to what one would expect.

I agree, the tests can stay, just need to flip the direction, as this assertion now passes.

Fixed in the next revision

How is it that these tests now pass? I would expect them not to pass. The rendered DOM in this test does contain the invalid html given. 🤔

HTML parser ignores unmatching closing tags. Currently we have this content: <span data-testid="child"></div>. After normalization via innerHTML it becomes <span data-testid="child"></span> and this matches the actual content.

With normalization, this test will not pass, for example:

const {container} = render(`<table> <tr> <td>test</td> </tr> </table>`) expect(container).toContainHTML('<table>test</table>');

<table>test</table> does not expand to the full table and does not match the real content

Hmmm, I wonder if we could detect that, and disallow invalid html. This also makes it technically a breaking change, although I'm thinking that I'll pass on that, since it is very unlikely someone was passing invalid html to explicitly expect a failure.

I think this is good to go. Will merge soon.

I am not sure this is possible without rolling your own parser. Built-in DOM parser does not expose enough information.

I am also wondering about the use-case here. Spotting the typos is nice, but is there any more important case where it is needed?

Not really, which is why I said I approve anyway. It's just a bit unintuitive, but I agree that it makes sense since it is how html works anyway. Developing our own parser is out of the question. This matcher is anyway an outlier, in the sense that we do not recommed its use except in some unique circumstances.

resolves testing-library#347

gnapse · 2021-03-25T15:54:29Z

@all-contributors please add @just-boris for code, test, bug

allcontributors · 2021-03-25T15:54:31Z

@gnapse

@just-boris already contributed before to code, test, bug

github-actions · 2021-03-25T15:57:49Z

🎉 This PR is included in version 5.11.10 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

just-boris commented Mar 18, 2021

View reviewed changes

gnapse self-assigned this Mar 19, 2021

just-boris force-pushed the main branch from 1392a1b to a6a7a6f Compare March 19, 2021 19:37

just-boris marked this pull request as ready for review March 19, 2021 19:37

gnapse reviewed Mar 23, 2021

View reviewed changes

fix: normalize expected value in toContainHTML

c0e8a1b

resolves testing-library#347

just-boris force-pushed the main branch from a6a7a6f to c0e8a1b Compare March 23, 2021 19:15

gnapse approved these changes Mar 24, 2021

View reviewed changes

gnapse merged commit 21ad89b into testing-library:main Mar 25, 2021

github-actions bot added the released label Mar 25, 2021

oponder mentioned this pull request Apr 7, 2021

Bump @testing-library/jest-dom from 5.11.9 to 5.11.10 giantswarm/happa#2314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: normalize expected value in toContainHTML #349

fix: normalize expected value in toContainHTML #349

just-boris commented Mar 18, 2021 •

edited

just-boris Mar 18, 2021

codecov bot commented Mar 18, 2021 •

edited

just-boris Mar 18, 2021

just-boris commented Mar 18, 2021 •

edited

gnapse commented Mar 19, 2021 •

edited

just-boris commented Mar 20, 2021

gnapse left a comment

gnapse Mar 23, 2021

just-boris Mar 23, 2021

gnapse Mar 23, 2021

just-boris Mar 23, 2021

gnapse Mar 24, 2021

just-boris Mar 25, 2021

gnapse Mar 25, 2021

gnapse commented Mar 25, 2021

allcontributors bot commented Mar 25, 2021

github-actions bot commented Mar 25, 2021

fix: normalize expected value in toContainHTML #349

fix: normalize expected value in toContainHTML #349

Conversation

just-boris commented Mar 18, 2021 • edited

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

just-boris commented Mar 18, 2021 • edited

gnapse commented Mar 19, 2021 • edited

just-boris commented Mar 20, 2021

gnapse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnapse commented Mar 25, 2021

allcontributors bot commented Mar 25, 2021

github-actions bot commented Mar 25, 2021

just-boris commented Mar 18, 2021 •

edited

codecov bot commented Mar 18, 2021 •

edited

just-boris commented Mar 18, 2021 •

edited

gnapse commented Mar 19, 2021 •

edited