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

[WIP] Replace jsdom with happy-dom testing environment to enable Modal tests (#461) #545

Open
wants to merge 2 commits into
base: bc/461
Choose a base branch
from

Conversation

bedrich-schindler
Copy link
Contributor

@bedrich-schindler bedrich-schindler commented May 31, 2024

This is work-in-progress pull request!

I've replaced jsdom with happy-dom testing environment and with little changes (typically because of use of incorrect jest functions ) and Modal tests with <dialog> works almost as expected. But for some reason, there are 4 tests in Grid.test.jsx that does not work. TBH, I don't know why.

~~I debbuged test functions, respectively toHaveStyle which is part of node_modules/@testing-library/jest-dom/dist/matchers-342a062d.js. toHaveStyle internally uses isSubset function`. I edited function to be able to debug it step-by-step:

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

      return spellingVariants.some(
        name => {
          const a = computedStyle[name];
          const b = computedStyle.getPropertyValue(name);
          const v = [];
          for (let i = 0; i < computedStyle.length; i++) {
            v.push(computedStyle.item(i));
          }

          return a === value ||
            b === value;
        },
      )
    })
  )
}

And for some reason, not all CSS properties are part of computed styles (variable v) and that's the reason why the tests fails.

Can anybody look at it? Can we discuss if this is the right way? Because without happy-dom, we won't be able to test our Modal component which is problem guys from LMC's Spirit Design System encountered. (cc @adamkudrna @crishpeen @literat).

If we want to have Modal tested, we either skip those tests for rowGap and columnGap or we can test it different way, for example using .outerHTML.contains(<css>). It is not perfect nor good, but it works at least.

@bedrich-schindler
Copy link
Contributor Author

bedrich-schindler commented May 31, 2024

The problem is in var() values. For exact values it works as expected.

See: jsdom/jsdom#3299

@bedrich-schindler
Copy link
Contributor Author

bedrich-schindler commented Jun 1, 2024

Because of missing support for var() in testing environment, I have updated Grid.test.jsx to compare styles using outerHTML.includes(<CSS>) - the only way how to test it now.

It ready for review.

A mentioned guys from Spirit team for inspiration as their Modal is missing tests.

Copy link
Member

@adamkudrna adamkudrna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can understand what's going on, a ✅ for me.

],
[
{ size: 'small' },
(rootElement) => expect(within(rootElement).getByRole('presentation')).toHaveClass('isRootSizeSmall'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could still be getByRole('dialog'), couldn't it?

Comment on lines +56 to +57
// While <dialog> component uses built-in browser dialog functionality for closing the dialog, we preserve this
// functionality due to lack of support for <dialog> in jsdom and happy-dom testing environments.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any jsdom/happy-dom issues to link to so we can remove this in future?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obrazek

Could you check this please?

@@ -85,8 +86,7 @@
"eslint-plugin-react": "^7.33.2",
"eslint-plugin-react-hooks": "^4.6.0",
"identity-obj-proxy": "^3.0.0",
"jest": "^29.6.4",
"jest-environment-jsdom": "^29.6.4",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for not forgetting to remove this 🙂.

Comment on lines +87 to +88
// Two following tests must used rootElement.outerHTML.includes() to test presence of CSS variables in the DOM,
// because the `toHaveStyle` matcher does not support CSS variables with var() function.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Two following tests must used rootElement.outerHTML.includes() to test presence of CSS variables in the DOM,
// because the `toHaveStyle` matcher does not support CSS variables with var() function.
// The following tests must use `rootElement.outerHTML.includes()` to test presence of CSS variables in the DOM
// because the `toHaveStyle` matcher does not support reading CSS variables via the `var()` function.

It's twice in this document.

Comment on lines +130 to +131
Object.entries(responsiveSpacingStyles('row-gap')).forEach(([ccsAttribute, cssValue]) => {
expect(rootElement.outerHTML.includes(`${ccsAttribute}: ${cssValue}`)).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Object.entries(responsiveSpacingStyles('row-gap')).forEach(([ccsAttribute, cssValue]) => {
expect(rootElement.outerHTML.includes(`${ccsAttribute}: ${cssValue}`)).toBeTruthy();
Object.entries(responsiveSpacingStyles('row-gap')).forEach(([cssAttribute, cssValue]) => {
expect(rootElement.outerHTML.includes(`${cssAttribute}: ${cssValue}`)).toBeTruthy();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants