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.

Sign up for GitHub

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

Switch away from using enzyme.mount in tests (components/higher-order/with-filters/test/index.js) #7793

Merged
merged 4 commits into from
Jul 10, 2018
Merged

Switch away from using enzyme.mount in tests (components/higher-order/with-filters/test/index.js) #7793

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe5c161
switch away from using enzyme.mount in tests
nerrad Jul 8, 2018
1522ec4
switch test to be against root node.
nerrad Jul 9, 2018
c7a5030
fix linting issues
nerrad Jul 9, 2018
edc4fdd
remove unnecessary unmount
nerrad Jul 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 65 additions & 21 deletions components/higher-order/with-filters/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,64 @@
/**
* External dependencies
*/
import { mount, shallow } from 'enzyme';
import { shallow } from 'enzyme';
import TestUtils from 'react-dom/test-utils';
import ReactDOM from 'react-dom';
Copy link
Member

Choose a reason for hiding this comment

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

I guess this shouldn't refer react-dom directly, but rather @wordpress/element. I'm not quite sure what is the best way to handle it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm sure, but then we'd be indirectly testing the export from @wordpress/element right? This is being used more as a test utility in this case?

Copy link
Member

Choose a reason for hiding this comment

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

It’s fine as is for the time being. We need to create our own lib anyways 😃


/**
* WordPress dependencies
*/
import { addFilter, removeAllFilters, removeFilter } from '@wordpress/hooks';
import { Component } from '@wordpress/element';

/**
* Internal dependencies
*/
import withFilters from '..';

const assertExpectedHtml = ( wrapper, expectedHTML ) => {
/* eslint-disable react/no-find-dom-node */
const element = ReactDOM.findDOMNode( wrapper );
/* eslint-enable react/no-find-dom-node */
expect( element.outerHTML ).toBe( expectedHTML );
};

// this is needed because TestUtils does not accept a stateless component.
// anything run through a HOC ends up as a stateless component.
const getTestComponent = ( WrappedComponent ) => {
class TestComponent extends Component {
render() {
return <WrappedComponent { ...this.props } />;
}
}
return <TestComponent />;
};

describe( 'withFilters', () => {
let wrapper;
let shallowWrapper, wrapper;

const hookName = 'EnhancedComponent';
const MyComponent = () => <div>My component</div>;

afterEach( () => {
wrapper.unmount();
if ( wrapper ) {
/* eslint-disable react/no-find-dom-node */
ReactDOM.unmountComponentAtNode( ReactDOM.findDOMNode( wrapper ).parentNode );
}
if ( shallowWrapper ) {
shallowWrapper.unmount();
}
/* eslint-enable react/no-find-dom-node */
removeAllFilters( hookName );
} );

it( 'should display original component when no filters applied', () => {
const EnhancedComponent = withFilters( hookName )( MyComponent );

wrapper = shallow( <EnhancedComponent /> );
shallowWrapper = shallow( <EnhancedComponent /> );

expect( wrapper.html() ).toBe( '<div>My component</div>' );
expect( shallowWrapper.html() ).toBe( '<div>My component</div>' );
shallowWrapper.unmount();
Copy link
Member

Choose a reason for hiding this comment

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

No need to do it, afterEach will handle it

} );

it( 'should display a component overridden by the filter', () => {
Expand All @@ -41,9 +70,9 @@ describe( 'withFilters', () => {
);
const EnhancedComponent = withFilters( hookName )( MyComponent );

wrapper = shallow( <EnhancedComponent /> );
shallowWrapper = shallow( <EnhancedComponent /> );

expect( wrapper.html() ).toBe( '<div>Overridden component</div>' );
expect( shallowWrapper.html() ).toBe( '<div>Overridden component</div>' );
} );

it( 'should display two components composed by the filter', () => {
Expand All @@ -60,9 +89,9 @@ describe( 'withFilters', () => {
);
const EnhancedComponent = withFilters( hookName )( MyComponent );

wrapper = shallow( <EnhancedComponent /> );
shallowWrapper = shallow( <EnhancedComponent /> );

expect( wrapper.html() ).toBe( '<div><div>My component</div><div>Composed component</div></div>' );
expect( shallowWrapper.html() ).toBe( '<div><div>My component</div><div>Composed component</div></div>' );
} );

it( 'should not re-render component when new filter added before component was mounted', () => {
Expand All @@ -82,12 +111,15 @@ describe( 'withFilters', () => {
);
const EnhancedComponent = withFilters( hookName )( SpiedComponent );

wrapper = mount( <EnhancedComponent /> );
wrapper = TestUtils.renderIntoDocument( getTestComponent( EnhancedComponent ) );
Copy link
Member

Choose a reason for hiding this comment

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

Can we use destructuring when importing TestUtils to avoid using the variable??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TestUtils is the default export from react-dom/test-utils. So I'd have to destructure on a subsequent statement. If renderIntoDocument was available as a direct import I'd definitely do it but its not. For now, I think it's a bit useful knowing at a glance that renderIntoDocument is part of TestUtils?

Copy link
Member

Choose a reason for hiding this comment

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

It’s quite surprising but good yo know 😃


jest.runAllTimers();

expect( spy ).toHaveBeenCalledTimes( 1 );
expect( wrapper.html() ).toBe( '<blockquote><div>Spied component</div></blockquote>' );
assertExpectedHtml(
wrapper,
'<blockquote><div>Spied component</div></blockquote>'
);
} );

it( 'should re-render component once when new filter added after component was mounted', () => {
Expand All @@ -98,7 +130,7 @@ describe( 'withFilters', () => {
};
const EnhancedComponent = withFilters( hookName )( SpiedComponent );

wrapper = mount( <EnhancedComponent /> );
wrapper = TestUtils.renderIntoDocument( getTestComponent( EnhancedComponent ) );

spy.mockClear();
addFilter(
Expand All @@ -113,7 +145,10 @@ describe( 'withFilters', () => {
jest.runAllTimers();

expect( spy ).toHaveBeenCalledTimes( 1 );
expect( wrapper.html() ).toBe( '<blockquote><div>Spied component</div></blockquote>' );
assertExpectedHtml(
wrapper,
'<blockquote><div>Spied component</div></blockquote>'
);
} );

it( 'should re-render component once when two filters added in the same animation frame', () => {
Expand All @@ -123,7 +158,7 @@ describe( 'withFilters', () => {
return <div>Spied component</div>;
};
const EnhancedComponent = withFilters( hookName )( SpiedComponent );
wrapper = mount( <EnhancedComponent /> );
wrapper = TestUtils.renderIntoDocument( getTestComponent( EnhancedComponent ) );

spy.mockClear();

Expand All @@ -148,7 +183,10 @@ describe( 'withFilters', () => {
jest.runAllTimers();

expect( spy ).toHaveBeenCalledTimes( 1 );
expect( wrapper.html() ).toBe( '<section><blockquote><div>Spied component</div></blockquote></section>' );
assertExpectedHtml(
wrapper,
'<section><blockquote><div>Spied component</div></blockquote></section>'
);
} );

it( 'should re-render component twice when new filter added and removed in two different animation frames', () => {
Expand All @@ -158,7 +196,7 @@ describe( 'withFilters', () => {
return <div>Spied component</div>;
};
const EnhancedComponent = withFilters( hookName )( SpiedComponent );
wrapper = mount( <EnhancedComponent /> );
wrapper = TestUtils.renderIntoDocument( getTestComponent( EnhancedComponent ) );

spy.mockClear();
addFilter(
Expand All @@ -179,7 +217,10 @@ describe( 'withFilters', () => {
jest.runAllTimers();

expect( spy ).toHaveBeenCalledTimes( 2 );
expect( wrapper.html() ).toBe( '<div>Spied component</div>' );
assertExpectedHtml(
wrapper,
'<div>Spied component</div>'
);
} );

it( 'should re-render both components once each when one filter added', () => {
Expand All @@ -190,12 +231,12 @@ describe( 'withFilters', () => {
};
const EnhancedComponent = withFilters( hookName )( SpiedComponent );
const CombinedComponents = () => (
<div>
<section>
<EnhancedComponent />
<EnhancedComponent />
</div>
</section>
);
wrapper = mount( <CombinedComponents /> );
wrapper = TestUtils.renderIntoDocument( getTestComponent( CombinedComponents ) );

spy.mockClear();
addFilter(
Expand All @@ -210,6 +251,9 @@ describe( 'withFilters', () => {
jest.runAllTimers();

expect( spy ).toHaveBeenCalledTimes( 2 );
expect( wrapper.html() ).toBe( '<div><blockquote><div>Spied component</div></blockquote><blockquote><div>Spied component</div></blockquote></div>' );
assertExpectedHtml(
wrapper,
'<section><blockquote><div>Spied component</div></blockquote><blockquote><div>Spied component</div></blockquote></section>'
);
} );
} );