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.

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

toHaveStyle does not compare empty values #272

Closed
just-boris opened this issue Jul 6, 2020 · 10 comments
Closed

toHaveStyle does not compare empty values #272

just-boris opened this issue Jul 6, 2020 · 10 comments
Labels
bug Something isn't working released

Comments

@just-boris
Copy link
Contributor

just-boris commented Jul 6, 2020

  • @testing-library/jest-dom version: 5.11.0
  • node version: 12.17.0
  • npm (or yarn) version: 6.14.4

Relevant code or config:

test('div max-width', () => {
  const div = document.createElement('div');
  div.style.maxWidth = '300px';
  div.style.minWidth = '200px';
  expect(div).toHaveStyle({ minWidth: '200px', maxWidth: '' }); // passes, but it should not
});

I expect that this assertion will fail, because maxWidth is not empty.

It works, when I am using toEqual instead of toHaveStyle

expect(div.style).toEqual(expect.objectContaining({ minWidth: '200px', maxWidth: '' }));

Suggested solution:

Somehow this information about empty values is lost around these lines:

const cssToParse = getCSStoParse(htmlElement.ownerDocument, css)
const parsedCSS = parseCSS(cssToParse, toHaveStyle, this)

A potential fix would be to check that all keys are from the expectation are defined on the element style definition.

@lourenci
Copy link
Contributor

lourenci commented Jul 6, 2020

It's working for me. Can you reproduce in a codesandbox, please?

image

@just-boris
Copy link
Contributor Author

Yes, reproduced in CodeSandbox: https://codesandbox.io/s/react-testing-library-demo-3c133?file=/src/__tests__/hello.js

it works fine if it was the only style property. When there are more, it gives false-positive. I also updated the original issue description

@nickmccurdy nickmccurdy added the bug Something isn't working label Jul 7, 2020
@lourenci
Copy link
Contributor

lourenci commented Jul 7, 2020

I can confirm the bug. I don't know how to solve it yet, but:

We underlie on the DOM API to parse the expected CSS object in a CSS before we compare it against the element's computed style, this API removes any invalid CSS attribute and it does not care about an "unset" attribute.

So in your example, the matcher ends up comparing partially against the element.

  const div = document.createElement('div');
  div.style.maxWidth = '300px';
  div.style.minWidth = '200px';
  expect(div).toHaveStyle({ minWidth: '200px', maxWidth: '' }); // the API will remove the empty string attribute (maxWidth) and it will match only against minWidth.

@just-boris
Copy link
Contributor Author

I don't understand, why we need to serialize styles to string and then parse it back, losing the information about empty but defined keys.

The color normalization is being done later in another function, why parseCSS is needed for object syntax?

@lourenci
Copy link
Contributor

lourenci commented Jul 7, 2020

I don't know if I got your question, but we need to normalize the expected value to match it against the element's css.

An element with the style background-color: red; min-width: 200px has to match against both toHaveStyles (no matter what is the order):

expect(element).toHaveStyle('min-width: 200px; background-color: red')
expect(element).toHaveStyle({ minWidth: '200px', backgroundColor: 'red' })

One function converts styled css in css text, another parses the css text in object like { background-color } to match it against the element's computedStyle.

@just-boris
Copy link
Contributor Author

just-boris commented Jul 7, 2020

Yes, parsing CSS makes sense for the CSS-as-string argument. However, for the object format, it seems redundant, because keys order usually does not affect object equality.

Is there a chance to refactor toHaveStyle implementation to make it just like expect(element.style).toEqual({...}) with extra colors normalization?

@lourenci
Copy link
Contributor

lourenci commented Jul 7, 2020

I got your point. I think it could work. Do you want to try it?

@just-boris
Copy link
Contributor Author

Yes, I will give it a try and come back with a PR if this works out.

@just-boris
Copy link
Contributor Author

The PR is out: #276

@gnapse gnapse closed this as completed in 5bea350 Jul 15, 2020
@gnapse
Copy link
Member

gnapse commented Jul 15, 2020

🎉 This issue has been resolved in version 5.11.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

No branches or pull requests

4 participants