feat: assert whether a string Element is in the document OR contained… #42

6ewis · 2018-07-24T00:54:43Z

What:

Assert whether a string representing a HTML Element is in the document: expect("<div>Example</div>").toBeInTheDocument()

Assert whether a string representing a HTML element is contained in another Element: expect(container).toContainElement(<div> Example </div>)

Why:

It's more intuitive to expect the methods toContainElement() and toBeInTheDocument() to expect HTML wrapped in strings. Also, there are use cases where it makes more sense. Picture a component that receives a dynamic HTML content as an input and parse it via a HTML parser - we could then test that the newly added DOM Element are in the document. It is more declarative - we are not only limited to query the DOM to check if an element is in the DOM.

How:

Simply By checking the type of the argument, if it is a string then we verify that it is a valid HTML element first then we check that it is included in the html DOM.

Checklist:

gnapse · 2018-07-24T14:13:36Z

This looks interesting. I wonder though if this would be resilient to subtle changes in the string representation of the element expected to be found. Also, how resilient is this to changes of whitespace in the string representation of the element to be found.

For instance, consider the following example:

// <div><h1>Hello</h1> ...more stuff</div>
expect('<h1>Hello</h1>').toBeInTheDocument();

Would the above test still pass after some code is introduced that attaches, for example, a data- attribute to that element? Or does the user need to provide an exact html representation of the element, with all its attributes?

// <div><h1 data-outline="1.1">Hello</h1> ...more stuff</div>
expect('<h1>Hello</h1>').toBeInTheDocument();

The other caveat I see could happen is with whitespace:

// <div><h1>  
//      Hello
//
// World
//     </h1> ...more stuff</div>
expect('<h1>Hello World</h1>').toBeInTheDocument();

Not saying it is wrong if any of the two examples above do not pass. I just wanted to bring it up for discussion, and if that's the case, and if this is to be merged, we need to document those caveats, or better yet, see if we can mitigate some of their effects.

kentcdodds · 2018-07-24T14:17:10Z

I worry that this would make the tests to tied to the DOM implemention. What if I put some text in a span? Doesn't change the way it looks, but it does break the test. I personally would not use it recommend this feature.

jgoz · 2018-07-24T14:55:46Z

I understand the motivation for this change, but I think its limitations make it too brittle, as @gnapse and @kentcdodds pointed out above. Maybe if the behaviour was closer to enzyme's containsMatchingElement and compared attribute subsets, was insensitive to whitespace, etc., it would be more broadly useful. But that is a lot more effort.

6ewis · 2018-07-24T15:31:57Z

@gnapse I think you made fair points, but isn't it it the same concerns with toHaveTextContent and white spaces?
@kentcdodds I don't find it to me more tied to the DOM implementation than the current implementation. Can you expand on your thoughts ? I'm not sure I follow. I came up with this after writing expect(container.outerHTML).toMatch() a dozen times in my use case, I wanted to make sure that whatever HTML my component receives is indeed in the DOM
@jgoz I don't believe that there's anything wrong by being inherently 'exact'

jgoz · 2018-07-24T15:55:35Z

@jgoz I don't believe that there's anything wrong by being inherently 'exact'

That's true, and there are other parts of this library that I wish were more exact by default (such as toHaveTextContent). My issue isn't directly with exactness, it's with the effects thereof — comparing the exact DOM representation of an element can lead to a lot of churn in your assertions as things evolve because changes that might be unrelated to particular tests are more likely to cause those tests to fail.

Another consequence of exactness, in this case, is that the tests are verifying details about the structure of your markup that should be irrelevant to the user. I.e., as a user, I don't care if the "user name" element uses a div or a span, I just care that it contains my username. You as the developer may care about the structure, but assertions like that go against the principles of the testing libraries that spawned jest-dom (react-testing-library, dom-testing-library).

6ewis · 2018-07-24T16:02:45Z

@jgoz let's agree to disagree. In my use case my user directly manipulates html from a platform called contentful. Not all users are created equal so quite the opposite it seems that I'm testing what they care about. They expect their html to be inserted in the DOM. I believe context matters, and generalizing that exactness is always testing implementation details is not a statement I agree with.

gnapse · 2018-07-24T17:19:30Z

@6ewis your use case sounds like you have components that receive html as a string (provided by a user in the real use of those components in your app) and they render that html within them, and you want to assert that they did so and used that html verbatim. Is that so? Sounds interesting!

It kinda puts our concerns to test, I have to admit. What we've all raised here is not to be easily dismissed though. jest-dom was spawned off react-testing-library, and the guiding principle behind it is to encourage testing from the perspective of how the user perceives the app in a browser. That's why testing against a specific DOM structure is usually considered counterproductive.

For example, the user normally cares to see a label saying "Your message was sent successfully!". They do not care if that message internally is a <div>Your message...</div> or if it's enclosed in a <span>. That's what we mean with our concerns, and that's why your feature request seems to go against those guiding principles.

It may be so that your use case is different, and we can discuss it. In your use case it may be valid, but we're worried that this new feature will be misused for use cases like the successful message notification above.

smacpherson64 · 2018-07-24T18:17:26Z

This is a really good conversation! The concept of how contentful is being used sounds awesome and is a fun use case! At first glance, it looks like a perfect opportunity for snapshot testing.

If the PR is merged, I prefer we keep the current interfaces, for consistency with the rest of the matchers, and don't overload the selectors to accept strings. Instead, I think a separate selector would be clearer: E.G.:

expect(container: HTMLElement | SVGElement).toContainHTML(htmlText: string)

gnapse · 2018-07-24T19:12:53Z

Hmmm, good point about snapshot testing. Wouldn't that work for you @6ewis?

Otherwise, the toContainHTML proposal is a very good idea. This would allow us to better document the caveats and warn about misuse, without affecting with such warnings the current less controversial use of the matchers being modified here.

…d in another Element

6ewis · 2018-07-25T02:34:44Z

@gnapse I shy away from snapshot testing in general because people tend to update large snapshots without paying much attention; I prefer more explicit assertions. More over, we cannot control what html the user will put - it's up to the users to decide whether their html is what they want or not, small or large.. We only sanitize the html.

Thank you guys for your input. I'll leave it to you to decide whether it's worth it to merge it or not. I like @smacpherson64 suggestions so I updated my PR and the documentation. However, it could also be good to see if it's a requested feature in the coming months. It may be too much of a rare case. The guiding principle of react testing lib took me here, so we should never lose sight of the big picture -- either way.

gnapse · 2018-07-25T15:50:27Z

src/__tests__/to-contain-html.js

+
+  expect(grandparent).toContainHtml(stringChildElement)
+  expect(parent).toContainHtml(stringChildElement)
+  expect(child).toContainHtml(stringChildElement)


It'd be nice to have .not.toContainHtml assertions here passing as well.

Also, I'd like to have tests about what happens when you pass in a string with invalid html, such as

expect(container).toContainHtml("<div>Hello world</p>");

What happens right now? Will the test simply not pass? Or will it not pass because it throws an error because the html is not valid? Regardless, that use case should be made clear in the tests.

There are also other cases, like what happens if you pass null or undefined instead of a string?

gnapse · 2018-07-25T15:53:54Z

src/to-contain-html.js

+
+export function toContainHtml(container, htmlText) {
+  const stringToHtml = item =>
+    new DOMParser().parseFromString(item, 'text/html').body.firstChild


This function stringToHtml should be declared above, outside this scope, as it does not need anything from this scope. Also prefer a function over an arrow function in this case. It could also take care of the if htmlText is a string check and return null.

gnapse · 2018-07-25T15:55:29Z

README.md

+toContainHtml(htmlText: String)
+```
+
+Assert whether a string representing a HTML element is contained in another Element:


Element at the end need not be capitalized.

gnapse · 2018-07-25T16:17:15Z

README.md

+```
+
+> Note: Chances are you probably do not need to use this method. We encourage testing from the perspective of how the user perceives the app in a browser. That's why testing against a specific DOM structure is not advised. It might come in handy in rare cases such as if your users manipulate the DOM such as a content management tool - where users are expected to type custom HTML. please use: [`toContainElement`](#tocontainelement)
+


I came up with an alternative that perhaps got too lengthy, so feel free to just take ideas from it. Cause I still think the text above is missing some key features:

Chances are you probably do not need to use this matcher. We encourage testing from the perspective of how the user perceives the app in a browser. That's why testing against a specific DOM structure is not advised.

It could be useful in situations where the code being tested renders html that was obtained from an external source, and you want to validate that that html code was used as intended.

It should not be used to check DOM structure that you control. A good rule of thumb would be to not use it if you find yourself passing a string literal as the expected html. In such cases please obtain actual references to the elements you want to check for (using document.querySelector or other equivalent methods) and use toContainElement instead.

jgoz

I like this better as a separate method. A few comments and suggestions from me.

jgoz · 2018-07-25T16:18:04Z

README.md

+### `toContainHtml`
+
+```typescript
+toContainHtml(htmlText: String)


String -> string

jgoz · 2018-07-25T16:21:25Z

README.md

+// ...
+```
+
+> Note: Chances are you probably do not need to use this method. We encourage testing from the perspective of how the user perceives the app in a browser. That's why testing against a specific DOM structure is not advised. It might come in handy in rare cases such as if your users manipulate the DOM such as a content management tool - where users are expected to type custom HTML. please use: [`toContainElement`](#tocontainelement)


~~You could probably leave out "It might come in handy in rare cases such as if your users manipulate the DOM such as a content management tool - where users are expected to type custom HTML".~~

~~And capital P on "please".~~

I wrote this before I saw @gnapse's feedback. I defer to his suggestion.

jgoz · 2018-07-25T16:22:38Z

src/to-contain-html.js

+import {matcherHint, printReceived} from 'jest-matcher-utils'
+import {checkHtmlElement} from './utils'
+
+export function toContainHtml(container, htmlText) {


This currently does not match the capitalization in the typescript definition. I suggest renaming everything to toContainHTML.

jgoz · 2018-07-25T16:24:58Z

src/to-contain-html.js

+import {checkHtmlElement} from './utils'
+
+export function toContainHtml(container, htmlText) {
+  const stringToHtml = item =>


You could move this out of toContainHTML and combine it with checkHtmlElement:

function checkHtmlText(htmlText, ...args) { const htmlElement = typeof htmlText === 'string' ? new DOMParser().parseFromString(htmlText, 'text/html').body.firstChild : null checkHtmlElement(htmlElement, ...args) }

And then use it instead of checkHtmlElement below:

checkHtmlElement(container, toContainHtml, this) checkHtmlText(htmlText, toContainHtml, this)

jgoz · 2018-07-25T16:31:58Z

src/__tests__/to-contain-html.js

@@ -0,0 +1,48 @@
+import {render} from './helpers/test-utils'
+
+/* eslint-disable max-statements */


Is this still needed?

jgoz · 2018-07-25T16:34:23Z

src/__tests__/to-contain-html.js

+
+  expect(grandparent).toContainHtml(stringChildElement)
+  expect(parent).toContainHtml(stringChildElement)
+  expect(child).toContainHtml(stringChildElement)


Can you add positive test cases for .not?

const nonExistantString = '<div data-testid="child"></div>' expect(grandparent).not.toContainHTML(nonExistantString) expect(parent).not.toContainHTML(nonExistantString) expect(child).not.toContainHTML(nonExistantString)

And then add the corresponding negative test cases below?

expect(() => expect(grandparent).toContainHTML(nonExistantString), ).toThrowError() // etc.

6ewis · 2018-07-25T18:05:07Z

@jgoz @gnapse updated 👍

gnapse

Looks good. Great suggestions from @jgoz. I have a couple more comments.

gnapse · 2018-07-25T18:45:19Z

README.md

+```
+
+> Chances are you probably do not need to use this matcher. We encourage testing from the perspective of how the user perceives the app in a browser. That's why testing against a specific DOM structure is not advised.
+


It's important to keep the > in front of these empty lines, so it all appears as a single note. The same applies to the next empty line below.

gnapse · 2018-07-25T18:49:08Z

src/__tests__/to-contain-html.js

+  const nonExistantElement = queryByTestId('not-exists')
+  const fakeElement = {thisIsNot: 'an html element'}
+  const stringChildElement = '<span data-testid="child"></span>'
+  const nonExistantString = '<span> Does not exists </span>'


(nit-picking alert!) should say "Does not exist" without the final "s".

Also, I still do not see any test about what happens if you pass invalid html, as in <div>hello</p>. I'm still not clear if in that case it fails because it won't find the invalid html in the container, or simply because it will fail to parse the invalid html in the first place.

6ewis · 2018-07-25T20:43:16Z

@gnapse anything else?

gnapse · 2018-07-25T21:40:49Z

README.md

 > It could be useful in situations where the code being tested renders html that was obtained from an external source, and you want to validate that that html code was used as intended.
-
+> \


I'm sorry for nit-picking, but why do you need the \? Without it you get separate praragraphs, which was my intention.

jgoz

Nice addition, thank you 👍

6ewis · 2018-07-26T00:08:42Z

@gnapse done

gnapse · 2018-07-26T13:38:25Z

🎉 This PR is included in version 1.11.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

feat: Assert whether a string representing a HTML element is containe…

2b3bf72

…d in another Element

6ewis force-pushed the pr/feat-string-HTML-inDOM branch from e36d36a to 2b3bf72 Compare July 25, 2018 02:24

fix: tests

2611ee3

gnapse requested changes Jul 25, 2018

View reviewed changes

jgoz requested changes Jul 25, 2018

View reviewed changes

Refactored - improved code as per feedbacks

fcc3b6a

fix - updated README

863da1d

gnapse reviewed Jul 25, 2018

View reviewed changes

Fix - updated Readme

f7ea428

gnapse reviewed Jul 25, 2018

View reviewed changes

jgoz approved these changes Jul 25, 2018

View reviewed changes

updated README

c9ca2e6

gnapse approved these changes Jul 26, 2018

View reviewed changes

gnapse merged commit 1df7560 into testing-library:master Jul 26, 2018

		```

		> Note: Chances are you probably do not need to use this method. We encourage testing from the perspective of how the user perceives the app in a browser. That's why testing against a specific DOM structure is not advised. It might come in handy in rare cases such as if your users manipulate the DOM such as a content management tool - where users are expected to type custom HTML. please use: [`toContainElement`](#tocontainelement)

		@@ -0,0 +1,48 @@
		import {render} from './helpers/test-utils'

		/* eslint-disable max-statements */

		```

		> Chances are you probably do not need to use this matcher. We encourage testing from the perspective of how the user perceives the app in a browser. That's why testing against a specific DOM structure is not advised.

		> It could be useful in situations where the code being tested renders html that was obtained from an external source, and you want to validate that that html code was used as intended.

		> \

feat: assert whether a string Element is in the document OR contained… #42

feat: assert whether a string Element is in the document OR contained… #42

Uh oh!

Conversation

6ewis commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnapse commented Jul 24, 2018

Uh oh!

kentcdodds commented Jul 24, 2018

Uh oh!

jgoz commented Jul 24, 2018

Uh oh!

6ewis commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgoz commented Jul 24, 2018

Uh oh!

6ewis commented Jul 24, 2018

Uh oh!

gnapse commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smacpherson64 commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnapse commented Jul 24, 2018

Uh oh!

6ewis commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgoz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgoz Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

6ewis commented Jul 25, 2018

Uh oh!

gnapse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

6ewis commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgoz left a comment

Choose a reason for hiding this comment

Uh oh!

6ewis commented Jul 26, 2018

Uh oh!

gnapse commented Jul 26, 2018

Uh oh!

Uh oh!

6ewis commented Jul 24, 2018 •

edited

Loading

6ewis commented Jul 24, 2018 •

edited

Loading

gnapse commented Jul 24, 2018 •

edited

Loading

smacpherson64 commented Jul 24, 2018 •

edited

Loading

6ewis commented Jul 25, 2018 •

edited

Loading

jgoz Jul 25, 2018 •

edited

Loading