-
Notifications
You must be signed in to change notification settings - Fork 7
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
base: bc/461
Are you sure you want to change the base?
Conversation
The problem is in See: jsdom/jsdom#3299 |
…enable `Modal` tests (#461)
Because of missing support for It ready for review. A mentioned guys from Spirit team for inspiration as their Modal is missing tests. |
There was a problem hiding this 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'), |
There was a problem hiding this comment.
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?
// 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -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", |
There was a problem hiding this comment.
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 🙂.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Object.entries(responsiveSpacingStyles('row-gap')).forEach(([ccsAttribute, cssValue]) => { | ||
expect(rootElement.outerHTML.includes(`${ccsAttribute}: ${cssValue}`)).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
This is work-in-progress pull request!
I've replacedjsdom
withhappy-dom
testing environment and with little changes (typically because of use of incorrect jest functions ) andModal
tests with<dialog>
works almost as expected. But for some reason, there are 4 tests inGrid.test.jsx
that does not work. TBH, I don't know why.~~I debbuged test functions, respectively
toHaveStyle
which is part ofnode_modules/@testing-library/jest-dom/dist/matchers-342a062d.js
.toHaveStyle
internally usesisSubset
function`. I edited function to be able to debug it step-by-step:And for some reason, not all CSS properties are part of computed styles (variablev
) and that's the reason why the tests fails.Can anybody look at it? Can we discuss if this is the right way? Because withouthappy-dom
, we won't be able to test ourModal
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.