-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Convert tests using enzyme to use react-test-renderer instead (components/inserter/test/menu.js) #7773
Convert tests using enzyme to use react-test-renderer instead (components/inserter/test/menu.js) #7773
Conversation
const activeCategory = wrapper.find( '.components-panel__body.is-opened > .components-panel__body-title' ); | ||
expect( activeCategory.text() ).toBe( 'Most Used' ); | ||
const activeCategory = wrapper.root.find( ( node ) => { | ||
return node.props.className && |
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.
- When will
className
be falsey? - This is more a perfect use case for
classList.contains
(assumingjsdom
supports it)
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.
Also this is prone to two issues:
- False negatives if order changes between application of
is-opened
andcomponents-panel__body
- False positives if suffixes/prefixes are applied to the class names we're testing (e.g.
not-the-components-panel__body is-opened-jk
)
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.
When will className be falsey?
find
traverses ALL components to verify there is only one match (throws an error as soon as there is more than one). So it's possible it hits a component where props.className
is undefined.
Good points on your subsequent comment though that need addressed.
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.
This is more a perfect use case for classList.contains (assuming jsdom supports it)
classList
is not exposed on the node
in this iterator. I'm a bit unclear on what you had in mind here. Were you thinking of a different approach instead of using TestRenderer.root.find
?
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.
False negatives if order changes between application of is-opened and components-panel__body
False positives if suffixes/prefixes are applied to the class names we're testing (e.g. not-the-components-panel__body is-opened-jk)
On re-examination, didn't those issues exist in the original test? I'm all for improving tests but the scope of this pull was a straight conversion. I'm open to also improving the test if thats the desire.
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.
Sorry, I mistook this as an HTMLElement, when in fact it's the React node.
On re-examination, didn't those issues exist in the original test? I'm all for improving tests but the scope of this pull was a straight conversion.
I don't think so. With the following diff applied, I'd expect the current tests to fail while the previous would have passed:
diff --git a/components/panel/body.js b/components/panel/body.js
index 1a22de279..048c3193d 100644
--- a/components/panel/body.js
+++ b/components/panel/body.js
@@ -40,7 +40,7 @@ class PanelBody extends Component {
const { title, children, opened, className, icon } = this.props;
const isOpened = opened === undefined ? this.state.opened : opened;
const arrow = `arrow-${ isOpened ? 'up' : 'down' }`;
- const classes = classnames( 'components-panel__body', className, { 'is-opened': isOpened } );
+ const classes = classnames( { 'is-opened': isOpened }, 'components-panel__body', className );
return (
<div className={ classes }>
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.
Well, it looks like the tests were since updated, but in the original implementation it would have been an issue.
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.
I expect the following would still result in a false positive:
diff --git a/components/panel/body.js b/components/panel/body.js
index 1a22de279..65a86abeb 100644
--- a/components/panel/body.js
+++ b/components/panel/body.js
@@ -40,7 +40,7 @@ class PanelBody extends Component {
const { title, children, opened, className, icon } = this.props;
const isOpened = opened === undefined ? this.state.opened : opened;
const arrow = `arrow-${ isOpened ? 'up' : 'down' }`;
- const classes = classnames( 'components-panel__body', className, { 'is-opened': isOpened } );
+ const classes = classnames( 'components-panel__body', className, { 'is-opened-not': isOpened } );
return (
<div className={ classes }>
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.
yup it would. revisiting.
return node.props.className && | ||
node.props.className.includes( 'components-panel__body-title' ); | ||
} ).find( ( node ) => { | ||
return node.type === 'button'; |
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.
Oof, this is a fair bit of effort to find a descendent node. I wonder if we could do for some of our small abstractions for common use cases.
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.
ya it ain't pretty. I'm wary of spending too much time on creating abstractions before these initial tests are converted (initial, being converting tests specifically surfacing problems with using enzyme). Once we get these tests in so that other dependent tickets can be adequately finished (i.e. 7557) then I think it'd be a good idea to tackle maybe doing our own abstraction library using TestUtils
for WP based jest tests so we can drop enzyme altogether. @gziolo and I have had a brief convo about that (but it should definitely be done in its own pull I think).
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.
Another option here would be to convert the component to a DOM node (reactDOM.findDOMNode
) and then use querySelector
on it. That will at least reduce this traversal to a single selector string similar to what was originally used.
6431b11
to
1852f3d
Compare
This allows for more accurately testing rendered expectations.
16a8ea5
to
85810b8
Compare
@aduth I rewrote these tests to use |
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.
Nice 👍
|
||
const initializeMenuDefaultStateAndReturnElement = ( propOverrides ) => { | ||
setWrapperForProps( propOverrides ); | ||
/* eslint-disable react/no-find-dom-node */ |
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.
We should consider overriding test files in ESLint to allow for findDOMNode
https://eslint.org/docs/user-guide/configuring#disabling-rules-only-for-a-group-of-files
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.
We should consider overriding test files in ESLint to allow for findDOMNode
That would be good, should be done in a separate pull though?
// NOTE: Due to https://github.com/airbnb/enzyme/issues/1174, some of the selectors passed through to | ||
// wrapper.find have had to be strengthened (and the filterWhere strengthened also), otherwise two | ||
// results would be returned even though only one was in the DOM. | ||
const defaultProps = { |
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.
This would fall under SCREAMING_SNAKE_CASE
constant convention:
https://github.com/WordPress/gutenberg/blob/master/docs/reference/coding-guidelines.md#constants
setTimeout: noop, | ||
}; | ||
|
||
let wrapper; |
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.
Doesn't need to be module scoped? Should this be the return value of setWrapperForProps
, then initializeMenuDefaultStateAndReturnElement
can internally assign it to a wrapper
variable if need be?
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.
Might also make setWrapperForProps
redundant altogether.
|
||
let wrapper; | ||
|
||
const setWrapperForProps = ( propOverrides ) => { |
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.
Honest curiosity: Why arrow functions vs. the function
keyword?
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.
habit mostly :)
- use SCREAMING_SNAKECASE for constant (standards) - refactor unnecessary module scoped variable
// results would be returned even though only one was in the DOM. | ||
const DEFAULT_PROPS = { | ||
position: 'top center', | ||
instanceID: 1, |
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.
withInstanceId
passes a prop called instanceId
, not instanceID
. Should we be concerned that this is not failing? Or do we not need it at all?
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.
ooo good catch! I wouldn't expect things to fail in the current tests because instanceId
is only implemented on the search label for
attribute and the search input id
attribute. We're not testing those rendered attributes specifically.
Still, this should probably be fixed by either removing instanceID from the props or just changing it to instanceId
. I would prefer removing it. I also only think this should be fixed to prevent confusion for other devs looking at this test, but really its not impacting existing tests (similar to how the lack of a rootUID
passed in as props on tests isn't affecting things.
Fix in a new pull?
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.
Yep, new pull sounds great. If we don't need it, removing it sounds best.
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.
Somewhat related. I wonder if this component should have defaultProps for implemented props that could be undefined if the component isn't wrapped (eg. instanceId
or rootUID
? Or is the unwrapped component just exported for convenience with testing?
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.
Or is the unwrapped component just exported for convenience with testing?
Primarily, yes.
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.
Description
This pull converts all usage of enzyme for tests in
components/inserter/test/menu.js
to usereact-test-renderer
instead. This is because enzyme does not support React 16.3+ (and movement to do so is really slow). This will fix issues with breakage due to the enzyme incompatibility as components receive React 16.3+ features (such asforwardRef
usage in #7557)Checklist: