Added unit test for select-dropdown component #9150

brunoscopelliti · 2016-11-04T07:26:41Z

Hi there,

I'm adding some unit tests for the select-dropdown component.

@gwwar, recently, you've reviewed my pull request #8915, would you mind to give me some advices to improve these tests. Feedback from other people is appreciated as well.

ps. I tried following this guide lines as much as possible, however it seems to me that the guide lines (last change November 2015) are now behind your current practices (enzyme's shallow vs React.addons.TestUtils just to make an example).

gwwar · 2016-11-04T22:02:19Z

client/components/select-dropdown/test/index.js

+ * External dependencies
+ */
+import React from 'react';
+


extra line here?

Yeah, that's an old habit of mine.
I like to group dependencies on the basis of their purpose; in this case I put that extra line to separate react, from the mocking/assertion libraries.

Since you don't have this convention, to let that extra line does not make sense in this case.

gwwar · 2016-11-04T22:04:48Z

client/components/select-dropdown/test/index.js

+import SelectDropdown from '../index';
+
+describe( 'index', function() {
+	require( 'test/helpers/use-fake-dom' )();


Unless we're running into load order issues, we should be able to do:

import useFakeDom from 'test/helpers/use-fake-dom'; //... describe( 'index', () => { useFakeDom();

I've verified, and it still works as expected... so it's ok for me to change.
Could you also please explain what you mean with

Unless we're running into load order issues

Did you have problems in the past, which were caused by requiring use-fake-dom at the top level?

Sure. In some cases when we pull in these dependencies they can interact with global data and mess with each other. I don't think there's something specific here with useFakeDom(), but maybe @gwwar was trying to figure out why it was a require() in-place instead of an import at the top level. I cannot speak for her though.

Thanks for the clarification.

why it was a require() in-place instead of an import at the top level

There's not a particular reason; I took the tests of the bulk-select component as example for many things (#8960 (comment)).
However, since having nested dependencies is not that common, I should have been more careful there.

Unfortunately one will likely be mislead by copying existing code around here. We're working on curating a list of good examples to use but for now, no harm done - that's what code review is for 😄

This is more of a Babel implementation detail at the moment, but require is synchronous while import is async. We generally only use require when importing dynamically (only in certain cases), or when we need to control the load order. For example mocking out packages when using use-mockery

gwwar · 2016-11-04T22:08:34Z

client/components/select-dropdown/test/index.js

+			const dropdownOptions = getDropdownOptions();
+			const dropdown = mount( <SelectDropdown options={ dropdownOptions } /> );
+
+			assert.equal( 'Statuses', dropdown.find( '.select-dropdown__label' ).text() );


I personally find expect to be a bit more readable but we use both styles in the codebase.

I agree that for specific assertions expect may lead to a more readable code.
At work we use QUnit, for personal projects I usually go for tape... so the assert style comes more natural to me.

I guess this could be the occasion to get more comfortable with expect.

My preference is with expect() and I'm pretty sure I see it far more in Calypso than assert

expect( dropdown.find( '.select-dropdown__label' ).text() ).to.eql( 'Statuses' );

I guess thatexpect is much more popular. Some test files use assert from Node or assert from Chai. It's all fine until there is only one of them used in a single file.

gwwar · 2016-11-04T22:09:08Z

client/components/select-dropdown/test/index.js

+import React from 'react';
+
+import { assert } from 'chai';
+import { shallow, mount } from 'enzyme';


Typically we prefer shallow to avoid the dom setup, but it looks like this component is complex enough to use mount.

Ok, I won't abuse of mount in the future.
I'll check if I can get equivalent assertions without mounting the whole component... otherwise I'll keep this one as is.

In first place shallow is much faster then mount and render.

gwwar · 2016-11-04T22:16:57Z

client/components/select-dropdown/test/index.js

+
+import { assert } from 'chai';
+import { shallow, mount } from 'enzyme';
+import sinon from 'sinon';


We also have the option of using a sinon sandbox so we can avoid having to manually restore stubs:

import { useSandbox } from 'test/helpers/use-sinon'; //... let sandbox; useSandbox( newSandbox => { sandbox = newSandbox; }); //... const stub = sandbox.stub( React.Component.prototype, 'setState' );

Yeah, I've noticed it during my exploration of your test base; it is essentially a mocha-aware wrapper around sinon.sandbox.create.
Are you suggesting it's better to use it even in a such simple scenario?

Also, I just noticed that for the assertions on spy/stub you never use sinon.assert.
I usually prefer sinon.assert cause it's more descriptive than chai, qunit (whatever...) when an assertion fails.
Do you see any problem?

Finally, I would ask @gziolo a question about use-sinon.
I noticed that the pattern:

let sandbox; useSandbox( newSandbox => { sandbox = newSandbox; });

is diffused through the whole testbase.

I was wondering why useSandbox does not just return the newly created sandbox; and noticed that this functionality was recently added (80727d5), and then removed (f37aa64#diff-c988aa06ee204545fc3c01efb3ffe8c6).

I think the idea was pretty cool; however the initial implementation had a flaw (and probably the reason you removed it) in that it returned an empty object when executing useSandbox. That was due to the fact that sinon.sandbox.create( config ); was called too late (when mocha triggers the before hook).

I tried a different implementation, and tests still pass.
If in the next days I prepare a pull request, do you mind to review it?

I ran into a similar question about the usage here. Are we storing the sandbox to keep it from being garbage-colelcted during the tests?

Yeah, I've noticed it during my exploration of your test base; it is essentially a mocha-aware wrapper around sinon.sandbox.create.
Are you suggesting it's better to use it even in a such simple scenario?

useSandbox takes care of tear down for you. It just simplifies a lot when you are using stubs and mocks extensively. It's fine to restore a stub inside the single test as it's done in this PR :) I don't have a preferred method here. In some cases it's also a good idea to use sinon.test which mixes both approaches.

Also, I just noticed that for the assertions on spy/stub you never use sinon.assert.
I usually prefer sinon.assert cause it's more descriptive than chai, qunit (whatever...) when an assertion fails.

Chai wraps sinon.assert so it should have similar handling. I'm not sure I have never used sinon.assert directly. We have developers with different levels of experience with unit testing. It definitely helps to have one style of writing assertions and one place to look for examples and documentation. That's why I think it's nice that we can use sinon-chai. On the other hand I don't see anything bad with using sinon.assert directly :)

I was wondering why useSandbox does not just return the newly created sandbox;

I'm not quite sure now, but most likely we wanted to have one style of using test helpers. It was the only helper that was returning a value. Feel free to open PR if you have an idea how to improve our code. 👍

gwwar · 2016-11-04T22:22:24Z

Thanks @brunoscopelliti This looks great! It does look like https://github.com/Automattic/wp-calypso/blob/master/docs/react-component-unit-testing.md is rather out of date! Thanks for the reminder to clean this up.

cc @alisterscott and @retrofox for any other behavior we should check for.
cc @gziolo for any other testing tips

brunoscopelliti · 2016-11-05T06:46:12Z

@gwwar Thanks for the immediate feedback.
I'll check your points, and try to improve accordingly!

dmsnell · 2016-11-06T00:27:36Z

client/components/select-dropdown/test/index.js

+			const dropdown = shallow( <SelectDropdown options={ dropdownOptions } /> );
+
+			assert.equal( 1, dropdown.find( '.select-dropdown' ).length );
+			assert.equal( 0, dropdown.find( '.select-dropdown.is-open' ).length );


just for fun, the expect version is expect( dropdown.find( '.select-dropdox' ) ).to.have.lengthOf( 1 );

alternatively, we could use other methods…

expect( dropdown.some( '.select-dropdown' ) ).to.be.true;

gziolo · 2016-11-07T07:50:57Z

It does look like https://github.com/Automattic/wp-calypso/blob/master/docs/react-component-unit-testing.md is rather out of date!

Yes, it's so out of date :(

gziolo · 2016-11-07T08:26:29Z

client/components/select-dropdown/test/index.js

+			expect( dropdown.find( '.select-dropdown__options li.select-dropdown__label' ).text() ).to.eql( 'Statuses' );
+			expect( dropdown.find( '.select-dropdown__options li.select-dropdown__option' ) ).to.have.lengthOf( 4 );
+
+			// a separator is rendered in place of any falsy option.


This comment gives me a hint that it deservers its own it block.

Good point. I'll update the PR soon.

gziolo · 2016-11-07T08:36:20Z

cc @gziolo for any other testing tips

We don't have too many component tests, because they are usually quite hard to write and maintain. Anyway, we should try to improve in this area, so thanks a lot for your great effort on this PR. Nicely done! 💯

gziolo · 2016-11-07T17:20:04Z

client/components/select-dropdown/test/index.js

+	describe( 'component rendering', function() {
+		it( 'should render a list with the provided options', function() {
+			const dropdownOptions = getDropdownOptions();
+			const dropdown = mount( <SelectDropdown options={ dropdownOptions } /> );


You can also extract those two lines into its own utility method to reduce some code duplication.

brunoscopelliti · 2016-11-08T05:36:45Z

Thanks for the time you've dedicated reviewing this pr, and answering my questions.
Let me know if there's anything else I can do here.

gziolo · 2016-11-08T06:10:54Z

client/components/select-dropdown/test/index.js

+		return mount( <SelectDropdown options={ dropdownOptions } /> );
+	}
+
+	function shallowRenderDropdown( props ) {


gziolo · 2016-11-08T06:28:13Z

client/components/select-dropdown/test/index.js

+						focus: sinon.spy()
+					}
+				},
+				activateItem: activateItemSpy,


You can inline sinon.spy calls to make it more concise here and in prepareFakeEvent.

gziolo · 2016-11-08T06:33:16Z

client/components/select-dropdown/test/index.js

+			expect( initialSelectedValue ).to.equal( 'drafts' );
+		} );
+
+		it( 'should return `undefined`, when there aren\'t options', function() {


We usually use double quotes to avoid backslash in strings like \':

"should return `undefined`, when there aren't options"

Both styles are acceptable atm, there was some discussion here:
#9037 (review)

gziolo · 2016-11-08T06:37:50Z

Looks good to me. Nice set of tests which execute really fast even when we use mount. I like it 👍

I left a couple of comments, that can be addressed before we merge. They are optional, if you have time to fix them go for it, otherwise I'm going to merge this PR as it is. Unfortunately I can't do it easily myself because you are using repository fork ;)

Changes post code review + more tests Linting Extract mountDropdown and shallowRenderDropdown methods Replace comment with test Avoid escaping Inlining sinon.spy call

gziolo · 2016-11-09T17:54:41Z

@brunoscopelliti thanks for working on it. I just merged your changes 🎉

matticbot added the OSS Citizen label Nov 4, 2016

mtias added the Components label Nov 4, 2016

lancewillett added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 4, 2016

gwwar reviewed Nov 4, 2016

View reviewed changes

dmsnell reviewed Nov 6, 2016

View reviewed changes

gziolo reviewed Nov 7, 2016

View reviewed changes

gziolo reviewed Nov 8, 2016

View reviewed changes

gziolo added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 8, 2016

gziolo added the Testing label Nov 8, 2016

Added unit test for select-dropdown component

01f2ea6

Changes post code review + more tests Linting Extract mountDropdown and shallowRenderDropdown methods Replace comment with test Avoid escaping Inlining sinon.spy call

brunoscopelliti mentioned this pull request Nov 8, 2016

Bug: select-dropdown remains in an undetermined state when used through keyboard #9206

Closed

gziolo merged commit ce4aeef into Automattic:master Nov 9, 2016

gziolo removed the [Status] Ready to Merge label Nov 9, 2016

brunoscopelliti deleted the test/select-dropdown branch November 10, 2016 04:54

gwwar mentioned this pull request Nov 14, 2016

Refactor: my-sites/sidebar/sidebar convert to es6 class #9177

Merged

gwwar mentioned this pull request Nov 15, 2016

EditGravatar: Add image editor step #9317

Merged

2 tasks

Added unit test for select-dropdown component #9150

Added unit test for select-dropdown component #9150

Conversation

brunoscopelliti commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar commented Nov 4, 2016

brunoscopelliti commented Nov 5, 2016

Choose a reason for hiding this comment

gziolo commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Nov 7, 2016

Choose a reason for hiding this comment

brunoscopelliti commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Nov 8, 2016

gziolo commented Nov 9, 2016

gwwar Nov 4, 2016 •

edited

Loading