Fix checkHtmlElement helper

[sheerun]

Answers #64

[gnapse]

Thanks a lot!

I realized this when I tried to adapt checkHtmlElement and the accompanying exception class to support validating that an element is of other element sub-classes as well (i.e. parameterize what element classes it validates against). In particular, with the new toHaveFormValues matcher introduced in #65, I need to validate that the element I receive is either a HTMLFormElement or a HTMLFieldSetElement.

I'll see if I have time to check all this and maybe add this new capability right here, or maybe I'll add it in a different PR so as not to delay this any further.

[sheerun]

If you do so, please parameterize by string names, not actual references, e.g. ['HTMLFieldSetElement'], otherwise it won't support custom jsdom

[gnapse]

Thanks! That was exactly the insight I was looking for.

[sheerun]

As for the failing test, new DOMParser().parseFromString(htmlText, 'text/html').body.firstChild returns element the is not attached to any window.. so it's not possible to verify it is HTMLElement of window it is attached to because element.ownerDocument.defaultView returns false

Why is exactly checkHtmlText needed? If I remove it all tests pass anyway

[sheerun]

I it was for me I'd remove this check instead of hacking around this issue. For me it seems it doesn't test for anything useful

[gnapse]

Interesting. I am too not convinced of the added complexity of what I was thinking to add to it. Or even the complexity that it has now. But it certainly serves a purpose in the clarity of how this failure is presented to the user.

Can you please tell me more: what would happen if people pass to expect(...) something that's not an element? Would you let it fail with the default error message of whatever happens first that does not conform to the argument being the expected type of element, or not an element at all?

[sheerun]

I'm talking about checkHtmlText, not checkHtmlElement

[gnapse]

Oh I see, let me check the code around that and recall what was it for.

[gnapse]

checkHtmlText is needed for the matcher toContainHtml. This matcher expects some html code in string form, and it confirms that that html structure is container in the passed container.

I think the reason behind it is to confirm that the html string passed to it is indeed valid html. The intention in that case would be for it to fail if you pass something like <div>Hello</span>. Not sure if that's the real purpose, of if it's even fulfilling it now.

I agree that you probably can remove it.

[sheerun]

Okay, I've removed it

[gnapse]

@sheerun can you provide more detail about why is this happening?

I'd like to understand the code in the future, and merely visiting the link did not make it evident what's happening here.

[sheerun]

I have no idea why, I know it happens and this try catch is needed for tests to pass

[gnapse]

But what do you mean with "Can throw for Document", and why do you link it to that issue in jsdom?

[sheerun]

It happens for this test where I pass Document instance: https://github.com/gnapse/jest-dom/pull/67/files#diff-f00ff29fba28ce3d11d8b6a726c3788eR51

The error I'm seeing when this test fails is the same as in the issue linked. I suspect this is issue with printWithType helper and internals of jsdom but it's not something I can fix easily.

[sheerun]

Maybe it's best if you remove this try catch and see what I mean

[sheerun]

Mind the error is still shown to the user but is little less descriptive as it doesn't show that Document instance has been passed

[gnapse]

🎉 This PR is included in version 2.1.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀