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

Adds basic Toolbar component tests. #937

Merged
merged 2 commits into from
Jun 2, 2017

Conversation

BE-Webdesign
Copy link
Contributor

Adds basic Toolbar component tests. Related to progress on #641.

Testing Instructions
Run npm i && npm run test-unit ensure tests pass. Change component logic
to ensure tests fail as they should.

@BE-Webdesign BE-Webdesign added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label May 30, 2017
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Thanks for working on these tests 👍

const toolbar = shallow( <Toolbar controls={ controls } /> );
const listItem = toolbar.find( 'IconButton' );
expect( toolbar.type() ).to.equal( 'ul' );
expect( listItem.props().icon ).to.equal( 'wordpress' );
Copy link
Member

Choose a reason for hiding this comment

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

Minor: listItem.prop( 'icon' ) more explicit. Same goes for most of the following lines. Else we could test:

expect( listItem.props() ).to.eql( {
	// ...keys
} );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought I updated these, guess not lol. Will change. Should it be changed to the one command style?

expect( listItem.props() ).to.eql( {
	// ...keys
} );

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a preference.

const toolbar = shallow( <Toolbar controls={ controls } /> );
const listItem = toolbar.find( 'IconButton' );
listItem.simulate( 'click', { stopPropagation: () => undefined } );
expect( clicked ).to.be.true();
Copy link
Member

Choose a reason for hiding this comment

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

Sinon spies are great for this sort of test:

http://sinonjs.org/releases/v2.3.2/spies/

Appears we're now using sinon-test as of #924, so it'd be matter of:

it( 'should call the clickHandler on click.', sinon.test( function() {
	const clickHandler = this.spy();
	// ...
	expect( clickHandler ).to.have.been.calledOnce;
} );

See also: https://github.com/domenic/sinon-chai

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I realized sinon was being used in our effects tests, so I will use that here as well in the revisions.

Adds basic Toolbar component tests. Related to progress on #641.

**Testing Instructions**
Run npm i && npm run test-unit ensure tests pass. Change component logic
to ensure tests fail as they should.
Cleans up the tests to make use of spies.  This also changes the
`props()` convention to check whether the keys are included in the
object.  include is used instead of equal to ensure that the tests do
not look at all props like children. Adds in a correction to the function
signature of Toolbar so that the component renders properly.
@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Jun 1, 2017

@iseulde If you could take a peek at this to make sure this aligns with the changes you made in the Toolbar component, it would be appreciated.

@BE-Webdesign BE-Webdesign merged commit 728c177 into master Jun 2, 2017
@youknowriad youknowriad deleted the add/test/components/toolbar branch June 2, 2017 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants