Fix unit tests

WordPress · Apr 19, 2019 · a8ffaa3 · a8ffaa3
1 parent 9fb41bb
commit a8ffaa3
Show file tree

Hide file tree

Showing 6 changed files with 85 additions and 107 deletions.
diff --git a/packages/block-editor/src/components/url-popover/test/__snapshots__/index.js.snap b/packages/block-editor/src/components/url-popover/test/__snapshots__/index.js.snap
@@ -4,7 +4,6 @@ exports[`URLPopover matches the snapshot in its default state 1`] = `
 <Popover
   className="editor-url-popover block-editor-url-popover"
   focusOnMount="firstElement"
-  noArrow={false}
   position="bottom center"
 >
   <div
@@ -28,7 +27,6 @@ exports[`URLPopover matches the snapshot when the settings are toggled open 1`]
 <Popover
   className="editor-url-popover block-editor-url-popover"
   focusOnMount="firstElement"
-  noArrow={false}
   position="bottom center"
 >
   <div
@@ -59,7 +57,6 @@ exports[`URLPopover matches the snapshot when there are no settings 1`] = `
 <Popover
   className="editor-url-popover block-editor-url-popover"
   focusOnMount="firstElement"
-  noArrow={false}
   position="bottom center"
 >
   <div

diff --git a/packages/components/src/dropdown-menu/test/index.js b/packages/components/src/dropdown-menu/test/index.js
@@ -14,7 +14,6 @@ import { Component } from '@wordpress/element';
  * Internal dependencies
  */
 import DropdownMenu from '../';
-import Popover from '../../popover';
 
 describe( 'DropdownMenu', () => {
 	let controls;
@@ -79,7 +78,7 @@ describe( 'DropdownMenu', () => {
 				}
 			);
 
-			expect( TestUtils.scryRenderedComponentsWithType( wrapper, Popover ) ).toHaveLength( 1 );
+			expect( TestUtils.scryRenderedDOMComponentsWithClass( wrapper, 'components-popover' ) ).toHaveLength( 1 );
 		} );
 	} );
 } );
diff --git a/packages/components/src/dropdown/test/index.js b/packages/components/src/dropdown/test/index.js
@@ -7,12 +7,11 @@ import TestUtils from 'react-dom/test-utils';
  * Internal dependencies
  */
 import Dropdown from '../';
-import Popover from '../../popover';
 
 describe( 'Dropdown', () => {
 	const expectPopoverVisible = ( wrapper, visible ) => {
 		expect(
-			TestUtils.scryRenderedComponentsWithType( wrapper, Popover ) )
+			TestUtils.scryRenderedDOMComponentsWithClass( wrapper, 'components-popover' ) )
 			.toHaveLength( visible ? 1 : 0 );
 	};
 	const buttonElement = ( wrapper ) => TestUtils.findRenderedDOMComponentWithTag(
@@ -31,13 +30,14 @@ describe( 'Dropdown', () => {
 				buttonElement( wrapper ).getAttribute( 'aria-expanded' )
 			).toBe( expanded.toString() );
 		};
+
 		const wrapper = TestUtils.renderIntoDocument( <Dropdown
 			className="container"
 			contentClassName="content"
 			renderToggle={ ( { isOpen, onToggle } ) => (
 				<button aria-expanded={ isOpen } onClick={ onToggle }>Toggleee</button>
 			) }
-			renderContent={ () => null }
+			renderContent={ () => <span>test</span> }
 		/> );
 
 		expectButtonExpanded( wrapper, false );

diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js
@@ -182,30 +182,39 @@ const Popover = ( {
 	}, [] );
 
 	// Focus handling
-	useRef( () => {
-		if ( ! focusOnMount || ! contentRef.current ) {
-			return;
-		}
+	useEffect( () => {
+		/*
+		 * Without the setTimeout, the dom node is not being focused. Related:
+		 * https://stackoverflow.com/questions/35522220/react-ref-with-focus-doesnt-work-without-settimeout-my-example
+		 *
+		 * TODO: Treat the cause, not the symptom.
+		 */
+		const focusTimeout = setTimeout( () => {
+			if ( ! focusOnMount || ! contentRef.current ) {
+				return;
+			}
 
-		if ( focusOnMount === 'firstElement' ) {
+			if ( focusOnMount === 'firstElement' ) {
 			// Find first tabbable node within content and shift focus, falling
 			// back to the popover panel itself.
-			const firstTabbable = focus.tabbable.find( contentRef.current )[ 0 ];
+				const firstTabbable = focus.tabbable.find( contentRef.current )[ 0 ];
+				if ( firstTabbable ) {
+					firstTabbable.focus();
+				} else {
+					contentRef.current.focus();
+				}
 
-			if ( firstTabbable ) {
-				firstTabbable.focus();
-			} else {
-				contentRef.current.focus();
+				return;
 			}
 
-			return;
-		}
-
-		if ( focusOnMount === 'container' ) {
+			if ( focusOnMount === 'container' ) {
 			// Focus the popover panel itself so items in the popover are easily
 			// accessed via keyboard navigation.
-			contentRef.current.focus();
-		}
+				contentRef.current.focus();
+			}
+		}, 0 );
+
+		return () => clearTimeout( focusTimeout );
 	}, [] );
 
 	// Event handlers

diff --git a/packages/components/src/popover/test/__snapshots__/index.js.snap b/packages/components/src/popover/test/__snapshots__/index.js.snap
@@ -1,6 +1,6 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`Popover #render() should pass additional props to portaled element 1`] = `
+exports[`Popover should pass additional props to portaled element 1`] = `
 <span>
   <div
     tabindex="-1"
@@ -23,7 +23,7 @@ exports[`Popover #render() should pass additional props to portaled element 1`]
 </span>
 `;
 
-exports[`Popover #render() should render content 1`] = `
+exports[`Popover should render content 1`] = `
 <span>
   <div
     tabindex="-1"

diff --git a/packages/components/src/popover/test/index.js b/packages/components/src/popover/test/index.js
@@ -2,110 +2,83 @@
  * External dependencies
  */
 import TestUtils from 'react-dom/test-utils';
-import ReactDOM from 'react-dom';
-import { noop } from 'lodash';
+
+/**
+ * WordPress dependencies
+ */
+import { Component } from '@wordpress/element';
 
 /**
  * Internal dependencies
  */
 import Popover from '../';
 
-describe( 'Popover', () => {
-	describe( '#componentDidUpdate()', () => {
-		let wrapper;
-		beforeEach( () => {
-			jest.spyOn( Popover.prototype, 'computePopoverPosition' ).mockImplementation( noop );
-			jest.spyOn( Popover.prototype, 'toggleAutoRefresh' ).mockImplementation( noop );
-		} );
+jest.useFakeTimers();
 
-		afterEach( () => {
-			jest.restoreAllMocks();
+class PopoverWrapper extends Component {
+	render() {
+		return <Popover { ...this.props } />;
+	}
+}
 
-			// Resetting keyboard state is deferred, so ensure that timers are
-			// consumed to avoid leaking into other tests.
-			jest.runAllTimers();
+describe( 'Popover', () => {
+	afterEach( () => {
+		if ( document.activeElement ) {
+			document.activeElement.blur();
+		}
+	} );
 
-			if ( document.activeElement ) {
-				document.activeElement.blur();
-			}
-		} );
+	it( 'should focus when opening in response to keyboard event', () => {
+		// As in the real world, these occur in sequence before the popover
+		// has been mounted. Keyup's resetting is deferred.
+		document.dispatchEvent( new window.KeyboardEvent( 'keydown' ) );
+		document.dispatchEvent( new window.KeyboardEvent( 'keyup' ) );
 
-		it( 'should turn on auto refresh', () => {
-			wrapper = TestUtils.renderIntoDocument( <Popover /> );
-			expect( Popover.prototype.toggleAutoRefresh ).toHaveBeenCalledWith( true );
-			expect( Popover.prototype.computePopoverPosition ).toHaveBeenCalled();
-		} );
+		// An ideal test here would mount with an input child and focus the
+		// child, but in context of JSDOM the inputs are not visible and
+		// are therefore skipped as tabbable, defaulting to popover.
+		let wrapper;
+		TestUtils.act( () => {
+			wrapper = TestUtils.renderIntoDocument( <PopoverWrapper /> );
 
-		it( 'should turn off auto refresh', () => {
-			wrapper = TestUtils.renderIntoDocument( <Popover /> );
-			/* eslint-disable react/no-find-dom-node */
-			ReactDOM.unmountComponentAtNode( ReactDOM.findDOMNode( wrapper ).parentNode );
-			/* eslint-enable react/no-find-dom-node */
+			jest.advanceTimersByTime( 1 );
 
-			expect( Popover.prototype.toggleAutoRefresh ).toHaveBeenCalledWith( false );
+			const content = TestUtils.findRenderedDOMComponentWithClass(
+				wrapper,
+				'components-popover__content'
+			);
+			expect( document.activeElement ).toBe( content );
 		} );
+	} );
 
-		it( 'should set offset and forced positions on changed position', () => {
-			const node = document.createElement( 'div' );
-			wrapper = ReactDOM.render( <Popover />, node );
-			jest.clearAllMocks();
+	it( 'should allow focus-on-open behavior to be disabled', () => {
+		const activeElement = document.activeElement;
+		TestUtils.act( () => {
+			TestUtils.renderIntoDocument( <Popover focusOnMount={ false } /> );
 
-			ReactDOM.render( <Popover position={ 'bottom right' } />, node );
+			jest.advanceTimersByTime( 1 );
 
-			expect( Popover.prototype.toggleAutoRefresh ).not.toHaveBeenCalled();
-			expect( Popover.prototype.computePopoverPosition ).toHaveBeenCalled();
+			expect( document.activeElement ).toBe( activeElement );
 		} );
+	} );
 
-		it( 'should focus when opening in response to keyboard event', ( done ) => {
-			// As in the real world, these occur in sequence before the popover
-			// has been mounted. Keyup's resetting is deferred.
-			document.dispatchEvent( new window.KeyboardEvent( 'keydown' ) );
-			document.dispatchEvent( new window.KeyboardEvent( 'keyup' ) );
-
-			// An ideal test here would mount with an input child and focus the
-			// child, but in context of JSDOM the inputs are not visible and
-			// are therefore skipped as tabbable, defaulting to popover.
-			wrapper = TestUtils.renderIntoDocument( <Popover /> );
-
-			setTimeout( () => {
-				const content = TestUtils.findRenderedDOMComponentWithClass(
-					wrapper,
-					'components-popover__content'
-				);
-				expect( document.activeElement ).toBe( content );
-				done();
-			}, 1 );
-
-			jest.runAllTimers();
+	it( 'should render content', () => {
+		let wrapper;
+		TestUtils.act( () => {
+			wrapper = TestUtils.renderIntoDocument( <PopoverWrapper>Hello</PopoverWrapper> );
 		} );
+		const content = TestUtils.findRenderedDOMComponentWithTag( wrapper, 'span' );
 
-		it( 'should allow focus-on-open behavior to be disabled', ( done ) => {
-			const activeElement = document.activeElement;
-
-			wrapper = TestUtils.renderIntoDocument( <Popover focusOnMount={ false } /> );
-
-			setTimeout( () => {
-				expect( document.activeElement ).toBe( activeElement );
-				done();
-			} );
-
-			jest.runAllTimers();
-		} );
+		expect( content ).toMatchSnapshot();
 	} );
 
-	describe( '#render()', () => {
-		it( 'should render content', () => {
-			const wrapper = TestUtils.renderIntoDocument( <Popover>Hello</Popover> );
-			const content = TestUtils.findRenderedDOMComponentWithTag( wrapper, 'span' );
-
-			expect( content ).toMatchSnapshot();
+	it( 'should pass additional props to portaled element', () => {
+		let wrapper;
+		TestUtils.act( () => {
+			wrapper = TestUtils.renderIntoDocument( <PopoverWrapper role="tooltip">Hello</PopoverWrapper> );
 		} );
+		const content = TestUtils.findRenderedDOMComponentWithTag( wrapper, 'span' );
 
-		it( 'should pass additional props to portaled element', () => {
-			const wrapper = TestUtils.renderIntoDocument( <Popover role="tooltip">Hello</Popover> );
-			const content = TestUtils.findRenderedDOMComponentWithTag( wrapper, 'span' );
-
-			expect( content ).toMatchSnapshot();
-		} );
+		expect( content ).toMatchSnapshot();
 	} );
 } );