toHaveStyle does not behave consistently between number and string values

[codepath2019]

• @testing-library/jest-dom version: 6.1.5
• node version: 20.3.1
• jest (or vitest) version: 0.34.1
• npm (or yarn) version: 9.6.7

Relevant code or config:

  it('non zero numbers passes style assertion for fontSize when conditions are not correct', () => {
    render(<div data-testid="element" style={{ fontSize: 8 }} />)
    expect(screen.getByTestId('element')).toHaveStyle({ fontSize: '8px' })
    expect(screen.getByTestId('element')).toHaveStyle({ fontSize: 8 })
    expect(screen.getByTestId('element')).not.toHaveStyle({ fontSize: 0 })
    expect(screen.getByTestId('element')).not.toHaveStyle({ fontSize: 1 })
  })

What you did:

I wrote the above test to understand how to assert on styles so that I can check that my fontSize values on my component is being configured correctly.

What happened:

The test above failed when fontSize has a number value and is above 0. I expected that providing a number or a pixel value should work exactly the same when asserting on styles.

Reproduction:

Problem description:

This is a problem because testers should have the confidence to be able to use between a number value or a string without worrying whether the test itself is correct.

Suggested solution:

[gnapse]

I'm not so sure that we should support a number value. It is not valid CSS to provide a numeric value to font-size, you have to provide a unit too:

Perhaps only font-size: 0 is supported.

The fact that it is supported in React could make me reconsider, but keep in mind that this is not a React testing framework. I'd rather leave it as it is, and use { fontSize: '8px' } in your tests. It is what React generates when you use the number value.

[codepath2019]

I understand that since the jest-dom matchers are not specifically built for React that it is less ideal to make it conform to what React handles. The problem remains that I am not able to document my test in the way I would consume the style object api. Not being able to write test the way I would interface with the api I develop with, in this case, React, makes for a funny in between place. What are alternatives that I can consider? Perhaps I can built my own toHaveReactStyles that lives in my code base? Thanks for taking the time to answer.

[gnapse]

I would consider supporting numbers as React do. Would you be open to make that contribution?

[codepath2019]

Hey @gnapse, I can give that a try. Is there a timeline on this? This will be my first time making code contribution to this repository but I am happy to taking it on.

[gnapse]

Is there a timeline on this?

Not sure what you mean by "timeline". If you mean that if there's a deadline, I'm not in a hurry to have this feature. You're the one who needs it.

I'm more than happy to help to guide you to make it happen. Submit a first draft PR, and I'll let you know what's missing in order to get it merged. Overall idea is:

• Make the change without breaking current use cases (existing tests should warn you about that)
• Add tests for the new behavior
• Document the new behavior in the README (with examples, at least)

[gnapse]

A caveat after giving it some thought

On a second thought, one thing that worries me is: how would this work in general? Not all properties should be converted to assume the unit is px. There are legitimate CSS properties that support a number (e.g. z-index: 1, which does not mean z-index: 1px). I wonder how React does it. I would not want to litter the code of this library with a huge list of exceptions for how certain CSS properties should be treated differently or supporting special cases. If things end up that way, I'd rather have you implement this first in your codebase, and if you end up with something generic and satisfactory enough, we could accept the contribution.

[codepath2019]

Thanks for bringing up the caveat. Since different CSS properties support different ranges of numbered values, it may prove to be difficult to capture all the exceptions. After a discussion with a colleague of mine, I believe jest-dom assertions should maintain the abstraction layer of the DOM and not the abstraction layer of what React provides. Here's what he said.

i don't think it's correct for React syntactic sugar expressions to become the defacto way that developers think, it blurs the lines between abstractions, and makes it so that React is now swaying tools that are more powerful in their pure state. React will die long before the current DOM implementation does. and future libs may decide, "hey this syntactic sugar is not that great, it obscures what's actually happening." e.g. the creator of NodeJS said he wishes he didn't make the "convenience" of being able to omit file extensions when importing in NodeJS, so Deno I believe requires file extensions.

[codepath2019]

One issue remains

The following test should fail, but it passes!

render(<div data-testid="element" style={{ fontSize: 8 }} />)
expect(screen.getByTestId('element')).toHaveStyle({ fontSize: 1 })

Would you agree @gnapse that we limit the scope of this issue to just this investigating and offering a solution PR for this bug? If so, would you like me to open a new issue just for this bug and close this one instead?

[codepath2019]

I went ahead and created a new issue so we can close this one b/c I don't think my original proposal of supporting number values for font-size the way react does is a good idea anymore. Instead, the new issue focuses on the bug itself. #564

[gnapse]

Thanks for bringing up the caveat. Since different CSS properties support different ranges of numbered values, it may prove to be difficult to capture all the exceptions. After a discussion with a colleague of mine, I believe jest-dom assertions should maintain the abstraction layer of the DOM and not the abstraction layer of what React provides. Here's what he said.

i don't think it's correct for React syntactic sugar expressions to become the defacto way that developers think, it blurs the lines between abstractions, and makes it so that React is now swaying tools that are more powerful in their pure state. React will die long before the current DOM implementation does. and future libs may decide, "hey this syntactic sugar is not that great, it obscures what's actually happening." e.g. the creator of NodeJS said he wishes he didn't make the "convenience" of being able to omit file extensions when importing in NodeJS, so Deno I believe requires file extensions.

I couldn't agree more with your colleague. He put in much better words my initial hesitation about this. Thanks for sharing.