testing-library / jest-dom

:owl: Custom jest matchers to test the state of the DOM
https://testing-library.com/docs/ecosystem-jest-dom
MIT License
4.44k stars 399 forks source link

toHaveStyle for property `font-size` behaves incorrectly for number values #564

Open codepath2019 opened 10 months ago

codepath2019 commented 10 months ago

Relevant code or config:

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

What you did:

I expected the above assertion to fail

What happened:

The test passes instead!✅

Reproduction:

Problem description:

the assertion is clearly incorrect and can easily cause bugs

Suggested solution:

I have not thought of a solution yet but would like to investigate this issue during my off hours.

EduardoSimon commented 8 months ago

Hi @gnapse, I've created a PR that addresses this issue. Please, feel free to comment or request any change, I'm a new contributor to the project (although a long time user 😃) and I'd like your feedback. Cheers.

ludacirs commented 8 months ago

I reside in a non-English-speaking region, so my English may be a bit clumsy. If I used incorrect vocabulary or seemed impolite, I apologize in advance.

While EduardoSimon's idea of failing the test in cases where an empty string occurs is valid, to understand why empty strings are occurring, I examined the behavior of CSSStyleDeclaration.

I found two issues:

  1. There is an issue in the getStyleDeclaration function where providing an incorrect value results in an empty string being added to the styles object.
  2. There is an issue in the computedStyle.getPropertyValue(property) function where using camelCase for property always returns an empty string.

1. getStyleDeclaration

function getStyleDeclaration(document, css) {
  const styles = {}

  const copy = document.createElement('div')
  Object.entries(css).forEach(([property, value]) => {
    // if value is incorrect
    copy.style[property] = value
    //  then styles[property] have "" value
    styles[property] = copy.style[property]
  })
  return styles
}

Returning an empty string when an incorrect value is provided might be considered expected.

Let's determine what is considered an incorrect value.

const copy = document.createElement('div');
copy.style.borderWidth = '1' // incorrect value => copy.style.borderWidth is ""
copy.style.borderWidth = 1 // incorrect value ""
copy.style.borderWidth = '1px' // correct value "1px"

copy.style.background = 'test' // incorrect value ""
copy.style.background = 1234 // incorrect value ""
copy.style.background = 'red' // correct value "red"

It seems that the rule for determining what is considered incorrect varies for each property.

A point here is that providing a number for borderWidth results in an empty string being returned.

2. CSSStyleDeclaration: getPropertyValue()

The getPropertyValue function returns an empty string when provided with camelCase.

const div = document.createElement('div');

div.style.borderWidth = '2px';

div.style.borderWidth // 2px
div.style.getPropertyValue('borderWidth') // ""
div.style.getPropertyValue('border-width') // 2px

Now that we have identified the two bugs, let's see how these bugs manifest in our tests.

As of the current version, 6.4.2, the following tests all pass…

// passed
const {container} = render(`<div class="border" style="border-width: 2px;/>`)
expect(container.querySelector('.border')).toHaveStyle({ borderWidth: '' })

// passed
const {container} = render(`<div class="border" style="border-width: 2px;/>`)
expect(container.querySelector('.border')).toHaveStyle({ borderWidth: 1 })

// passed
const {container} = render(`<div class="border" style="border-width: 2px;/>`)
expect(container.querySelector('.border')).toHaveStyle({ borderWidth: '2' })

Let's examine the flow based on the second test case.

The getStyleDeclaration function transforms our style object passed to toHaveStyle.

function getStyleDeclaration(document, css) {
  const styles = {}

  // css { borderWidth: 1 } => Remember, providing a number was considered an incorrect value
  const copy = document.createElement('div')
  Object.entries(css).forEach(([property, value]) => {
    copy.style[property] = value
    styles[property] = copy.style[property]
  })
  // styles {borderWidth: ''}
  return styles
}

Consequently, borderWidth became an empty string.

Next, the determination of test passage in isSubset is examined.

// the styles object is our styles returned by the getStyleDeclaration({ borderWidth: '' }).

// The computedStyle object is the CSSStyleDeclaration object of the `<div class="border" style="border-width: 2px;"/>` rendered in the test code.

function isSubset(styles, computedStyle) {
  return (
    !!Object.keys(styles).length &&
    Object.entries(styles).every(([prop, value]) => {
      const spellingVariants = [prop]
      if (!isCustomProperty(prop)) spellingVariants.push(prop.toLowerCase())

      return spellingVariants.some(name => {
        return (
          computedStyle[name] === value ||
          **computedStyle.getPropertyValue(name) === value**
          // Here, getPropertyValue returns an empty string,
          // and the value is also an empty string. Therefore, it returns true.
        )
      })
    })
  )
}

// prop, name
// ⇒ borderWidth

// value
// ⇒ ‘’ // because first case

// computedStyle[name]
// ⇒ 2px

// computedStyle.getPropertyValue(name)
// ⇒ ‘’ // because second case

// Therefore, the comparison values for spellingVariants.some(~) are as follows:
// computedStyle[name] === value
// 2px === ‘’ ⇒ false
// computedStyle.getPropertyValue(name) === value
// ‘’ === ‘’ ⇒ true

We can see why the test that should fail is passing.


@gnapse Can you review and fix this bug if I submit a pull request with the changes?