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

fix: normalize expected value in toContainHTML #349

Merged
merged 1 commit into from
Mar 25, 2021
Merged

fix: normalize expected value in toContainHTML #349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions src/__tests__/to-contain-html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,22 @@ describe('.toContainHTML', () => {
const nonExistantElement = queryByTestId('not-exists')
const fakeElement = {thisIsNot: 'an html element'}
const stringChildElement = '<span data-testid="child"></span>'
const stringChildElementSelfClosing = '<span data-testid="child" />'
const incorrectStringHtml = '<span data-testid="child"></div>'
const nonExistantString = '<span> Does not exists </span>'
const svgElement = queryByTestId('svg-element')

expect(grandparent).toContainHTML(stringChildElement)
expect(parent).toContainHTML(stringChildElement)
expect(child).toContainHTML(stringChildElement)
expect(child).toContainHTML(stringChildElementSelfClosing)
expect(grandparent).not.toContainHTML(nonExistantString)
expect(parent).not.toContainHTML(nonExistantString)
expect(child).not.toContainHTML(nonExistantString)
expect(child).not.toContainHTML(nonExistantString)
expect(grandparent).not.toContainHTML(incorrectStringHtml)
expect(parent).not.toContainHTML(incorrectStringHtml)
expect(child).not.toContainHTML(incorrectStringHtml)
expect(child).not.toContainHTML(incorrectStringHtml)
Comment on lines -32 to -35
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fixed in the next revision

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

// negative test cases wrapped in throwError assertions for coverage.
expect(() =>
Expand All @@ -59,6 +60,9 @@ describe('.toContainHTML', () => {
expect(() =>
expect(child).not.toContainHTML(stringChildElement),
).toThrowError()
expect(() =>
expect(child).not.toContainHTML(stringChildElementSelfClosing),
).toThrowError()
expect(() => expect(child).toContainHTML(nonExistantString)).toThrowError()
expect(() => expect(parent).toContainHTML(nonExistantString)).toThrowError()
expect(() =>
Expand All @@ -72,16 +76,16 @@ describe('.toContainHTML', () => {
expect(grandparent).toContainHTML(nonExistantElement),
).toThrowError()
expect(() =>
expect(nonExistantElement).toContainHTML(incorrectStringHtml),
expect(nonExistantElement).not.toContainHTML(incorrectStringHtml),
).toThrowError()
expect(() =>
expect(grandparent).toContainHTML(incorrectStringHtml),
expect(grandparent).not.toContainHTML(incorrectStringHtml),
).toThrowError()
expect(() =>
expect(child).toContainHTML(incorrectStringHtml),
expect(child).not.toContainHTML(incorrectStringHtml),
).toThrowError()
expect(() =>
expect(parent).toContainHTML(incorrectStringHtml),
expect(parent).not.toContainHTML(incorrectStringHtml),
).toThrowError()
})

Expand Down
12 changes: 11 additions & 1 deletion src/to-contain-html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
import {checkHtmlElement} from './utils'

function getNormalizedHtml(container, htmlText) {
const div = container.ownerDocument.createElement('div')
div.innerHTML = htmlText
return div.innerHTML
}

export function toContainHTML(container, htmlText) {
checkHtmlElement(container, toContainHTML, this)

if (typeof htmlText !== 'string') {
throw new Error(`.toContainHTML() expects a string value, got ${htmlText}`)
}
Comment on lines +12 to +14
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return {
pass: container.outerHTML.includes(htmlText),
pass: container.outerHTML.includes(getNormalizedHtml(container, htmlText)),
message: () => {
return [
this.utils.matcherHint(
Expand Down