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 for property font-size behaves incorrectly for number values #564

Open
codepath2019 opened this issue Dec 27, 2023 · 2 comments · May be fixed by #582
Open

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

codepath2019 opened this issue Dec 27, 2023 · 2 comments · May be fixed by #582
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@codepath2019
Copy link

  • @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:

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.

@gnapse gnapse added bug Something isn't working good first issue Good for newcomers labels Dec 28, 2023
EduardoSimon added a commit to EduardoSimon/jest-dom that referenced this issue Feb 16, 2024
Given an invalid declaration such as `fontSize: 8`, due to the missing
unit, the `toHaveStyle` matcher should not pass the following test:

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

This PR fixes testing-library#564 by adding a more restrictive guard in the matcher's
logic.
EduardoSimon added a commit to EduardoSimon/jest-dom that referenced this issue Feb 16, 2024
Given an invalid declaration such as `fontSize: 8`, due to the missing
unit, the `toHaveStyle` matcher should not pass the following test:

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

This PR fixes testing-library#564 by adding a more restrictive guard in the matcher's
logic.
@EduardoSimon EduardoSimon linked a pull request Feb 16, 2024 that will close this issue
4 tasks
@EduardoSimon
Copy link

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
Copy link

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?

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

Successfully merging a pull request may close this issue.

4 participants