Fix toHaveStyle not to return true when shouldn't

[CarlaTeo]

What:
As the docs says, the style property of an HTMLElement should not be directly assigned:

Styles should not be set by assigning a string directly to the style property (as in elt.style = "color: blue;"), since it is considered read-only, as the style attribute returns a CSSStyleDeclaration object which is also read-only. Instead, styles can be set by assigning values to the properties of style.

This probably is related to the issue: #68

Why:
By directly assigning style, whenever a nonexistent style was passed to getStyleDeclaration, it would return an empty object, making toHaveStyle always return true in these cases.

How:
Since the goal was to obtain a key value pair of style properties, getStyleDeclaration was changed to parse the string and obtain an object.
That object could be returned, but the HTMLElement style attribution was kept to normalize colors.

Checklist:

• Documentation "N/A"
• [ x] Tests
• Updated Type Definitions "N/A"
• [ x] Ready to be merged
• [ x] Added myself to contributors table

[gnapse]

Thanks! This looks good, though I left a couple of comments for your consideration.

[kevinSuttle]

I'm seeing this fail with both single and compound styles.

const {getByText} = render(<Button label="Default" />);
expect(getByText('Default')).toHaveStyle(
      'backgroundcolor: LITERALLY ANYTHING'
    );
// CLI: 👍 ✅  

[gnapse]

I'm seeing this fail with both single and compound styles.

const {getByText} = render(<Button label="Default" />);
expect(getByText('Default')).toHaveStyle(
      'backgroundcolor: LITERALLY ANYTHING'
    );
// CLI: 👍 ✅  

@kevinSuttle I'm not sure I understand your comment.

What are you seeing fail? The version of this library in this PR? Or are you referring to this library's currently stable release?

What about that thumbs-up and checkmark at the end of your comment alongside the "CLI" word? What does that mean? Because it seems to convey that things are working, not failing.

[dmwyatt]

@CarlaTeo Any chance you can look into getting this fixed up?

[nealoke]

Please merge this in, now any test with a invalid value just passes... 😢

[gnapse]

I'll take a look again at this towards the end of the week and I'll address the issues I mentioned in the code review. Sorry for the extra delay.

[weyert]

You can use Cypress for this? See: https://docs.cypress.io/guides/references/assertions.html#CSS
My understanding was that jsdom doesn't interpret the styling? That's why I am using Cypress to assert these kind of things.

[gnapse]

My understanding was that jsdom doesn't interpret the styling?

It does. But it's tricky. See this comment for what I mean. Overall yes, for advanced stuff that depends on styles normally acquired via a stylesheet, I'd use something like Cypress. jest-dom's style helpers could still help for styles applied directly to the elements via (e.g. in React having something like <div style={{ display: visible ? undefined : 'none' }}> would work with checking the display css rule value without much issues).

However, this ticket is still something we should address, regardless of the difficulties with working with styles in jsdom in general.

[CarlaTeo]

Sorry for the delay, people. 🙈

[klaaspieter]

I'm having issues with toHaveStyle as well. Anything I can do to get this merged?

[gnapse]

@klaaspieter my apologies for not having followed up on this as I said I would. Does this PR as it is covers the issues you're having with .toHaveStyles? I'm going to be merging it now, I just wanted to make sure the issues you mention are the ones this PR solves.

[gnapse]

🎉 This PR is included in version 4.1.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[klaaspieter]

@gnapse I didn't test it with my own changes but I think at a minimum this PR fixes a false positive. I was trying to test for this:

expect(element).toHaveStyle('-webkit-overflow-scrolling: touch')

If I recall correctly before this PR this would falsely claim be considered true because parsing the expected CSS resulted in {} and looking for the -webkit-overflow-scrolling key on element also resulted in {} (I think because JSDom doesn't support the vendor prefix it's never added to the element).

Conversely expect(element).not.toHaveStyle('-webkit-overflow-scrolling: touch') failed with an error saying that there is no difference between the expected and actual result because they're both {}. In this case the .not.toHaveStyle should actually pass.

I think this PR fixes that problem but I'd have to test it again to make sure.

[gnapse]

I think this PR may have introduced a problem. This now does not pass:

const { container } = render(<div style={{ border: 'none' }} />)
expect(container.firstChild).toHaveStyle('border: none')

Could it be? If so, what could we do about it?

[CarlaTeo]

I think this PR may have introduced a problem. This now does not pass:

const { container } = render(<div style={{ border: 'none' }} />)
expect(container.firstChild).toHaveStyle('border: none')

Could it be? If so, what could we do about it?

I am not sure about what problem you meant...I tried the following test:

const { container } = render(`<div class="label" style="border: 'none'" />`);
expect(container.querySelector('.label')).toHaveStyle('border: none')

And it does pass.

In fact, I realized this PR solves the case of nonexistant keys, but not values, since:

const { container } = render(<div style={{ border: 'none' }} />)
expect(container.firstChild).toHaveStyle('border: anything')

Still passes...

[gnapse]

Yeah, never mind, I had updated an old project to the latest version of jest-dom, and now a test similar to what I depicted above does not pass, failing at this style rule precisely. But I cannot reproduce in a minimal react app repo. False alarm.