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() doesn't know about aspect-ratio #570

Open
Fullchee opened this issue Jan 24, 2024 · 6 comments · May be fixed by #584
Open

.toHaveStyle() doesn't know about aspect-ratio #570

Fullchee opened this issue Jan 24, 2024 · 6 comments · May be fixed by #584
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@Fullchee
Copy link

Fullchee commented Jan 24, 2024

  • @testing-library/jest-dom version: 6.3.0
  • node version: 20.9.0
  • jest version: 27.5.1 (from create react app)
  • npm version: 10.1.0

Relevant code or config:

function App() {
  return <div style={{ aspectRatio: "1 / 1" }}>Hello</div>;
}

  it("should have an aspect-ratio", () => {
    render(<App />);
    const app = screen.getByText("Hello");
    expect(app).toHaveStyle({ "aspect-ratio": "1 / 1" });
    expect(app).toHaveStyle({ aspectRatio: "1 / 1" });
  });

What you did:

npm test

What happened:

✕ should have an aspect-ratio (47 ms)

  ● <App> › should have an aspect-ratio

    expect(element).toHaveStyle()

    - Expected

    - aspect-ratio: 1 / 1;

      11 |     - Expected
      12 |     - aspect-ratio: 1 / 1; */
    > 13 |     expect(app).toHaveStyle({ "aspect-ratio": "1 / 1" });
         |                 ^
      14 |     expect(app).toHaveStyle({ aspectRatio: "1 / 1" });
      15 |   });
      16 |

      at Object.<anonymous> (src/App.test.js:13:17)

A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.
Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total

Reproduction:

https://github.com/Fullchee/jest-dom-to-have-style

Problem description:

It works for other CSS properties, like color: blue

Suggested solution:

It looks like the dependency@adobe/css-tools can handle aspect-ratio so I don't think it's that

I'm not sure what defaultView is here

const {getComputedStyle} = htmlElement.ownerDocument.defaultView

@gnapse gnapse added bug Something isn't working good first issue Good for newcomers labels Jan 31, 2024
@fpapado
Copy link
Contributor

fpapado commented Feb 1, 2024

I did a quick debugging out of curiosity, since I've run into this in the past 😅

I have a gut feeling that this is not an issue in this library per se, but rather in the jsdom environment. Here is a reduced test case, creating the elements ad-hoc in the test.

test('aspect-ratio style declaration', () => {
  const el = document.createElement('div');
  el.style.aspectRatio = '1 / 1';
  el.style.width = '100%';
  document.body.appendChild(el);

  expect(el).toBeInTheDocument();

  // Uncomment for testing
  // console.log('Computed style', getComputedStyle(el));
  // console.log('Attribute style', el.style);

  // These pass
  // Barebones assertion
  expect(getComputedStyle(el).width).toEqual('100%');

  // @testing-library/jest-dom
  expect(el).toHaveStyle({ width: '100%' });

  // These fail
  // Barebones assertion
  expect(getComputedStyle(el).aspectRatio).toEqual('1 / 1');

  // @testing-library/jest-dom
  expect(el).toHaveStyle({ aspectRatio: '1 / 1' });
});

The logs themselves point that aspectRatio gets discarded in the computed style calculation, but is preserved in the one from .style. My guess is that jsdom normalises the values for computed style, but leaves inline styles intact. So it could be that they have not implemented that property yet 🤔

I checked jsdom's issue tracker and did not find anything relevant. My tentative suggestion is to raise the issue there, but I might be wrong!

  console.log
    Computed style CSSStyleDeclaration {
      '0': 'display',
      '1': 'visibility',
      '2': 'width',
      _values: { display: 'block', visibility: 'visible', width: '100%' },
      _importants: { display: '', visibility: undefined, width: '' },
      _length: 3,
      _onChange: [Function (anonymous)]
    }

      at Object.log (src/client/js/modules-v2/authModal/components/AuthModal/AuthModal.spec.tsx:1299:11)

  console.log
    Attribute style CSSStyleDeclaration {
      '0': 'width',
      _values: { width: '100%' },
      _importants: { width: undefined },
      _length: 1,
      _onChange: [Function (anonymous)],
      aspectRatio: '1 / 1'
    }

@adobe/css-tools mostly comes in when [parsing/normalising the declarations that the author provides to .toHaveStyle](https://github.com/testing-library/jest-dom/blob/c5c4e8d4c806caf9b1077dbfd155bd4520d9b02b/src/to-have-style.js#L59). The actual styles are directly from getComputedStyle.

@gnapse
Copy link
Member

gnapse commented Feb 7, 2024

Thanks, @fpapado, for taking such a close look at this.

I, too, suspect of jsdom. If only because the custom matcher does not do anything that is particular to this CSS property, but even more so because of your findings.

@Fullchee
Copy link
Author

It seems to be fixed in the latest versions of jsdom

When I install jsdom v16 (the same version listed in the package.json dev dependencies), I can reproduce the issue

When I install jsdom v24 (the latest), it doesn't have that issue.

https://github.com/Fullchee/jsdom-stripping-aspect-ratio


So the solution is to upgrade the jsdom version.

Note that this is a breaking change where only Node 18+ is supported on the latest version of jsdom

The version of jsdom to upgrade to will depend on which version of Node jest-dom wants to support in the next major release

@Fullchee
Copy link
Author

I noticed that jest-dom v6 dropped support anything before Node 14

https://github.com/testing-library/jest-dom/releases/tag/v6.0.0

and the latest version of jsdom that supports Node v14 is v21 (v22 min Node version is v16)

Let me see if this gets fixed in jsdom v21

@Fullchee
Copy link
Author

Fullchee commented Feb 18, 2024

Yup!

https://github.com/Fullchee/jsdom-stripping-aspect-ratio/tree/jsdom-v21-node-14

It looks it's fixed in v21! 🎉

Gonna create a PR!

@Fullchee Fullchee linked a pull request Feb 18, 2024 that will close this issue
1 task
@Fullchee
Copy link
Author

I'm stumped 😞

It looks like the aspect-ratio test fails in the PR where I upgrade to jsdom@21

https://github.com/testing-library/jest-dom/pull/584/files?w=1#diff-6006ecc8e0f5f628f890cb05deb67d73b6aa124dd95f8a718ab5d3dde88a96a0

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.

3 participants