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

Jump to bottom

Add tests for the UrlInputButton component. #3550

Merged

gziolo merged 8 commits into WordPress:master from hideokamoto:add/test/url-input/button

Nov 21, 2017

Contributor

hideokamoto commented Nov 18, 2017 •

edited

Loading

Description

Related to progress on the issue -> Unit Testing Components #641

Added some unit test scripts for UrlInputButton component.

How Has This Been Tested?

Run unit test command.

$ npm test
...
 PASS  blocks/url-input/test/button.js
  UrlInputButton
    ✓ should has valid class in wrapper tag (13ms)
    ✓ should not have is-active class if url prop not defined (15ms)
    ✓ should have is-active class if url prop defined (7ms)
    ✓ should hidden form for default (6ms)
    ✓ should visible form if Edit Link button clicked (15ms)
    ✓ should call onChange function at once if value changes at once (7ms)
    ✓ should call onChange function at twice if value changes at twice (4ms)
    ✓ should close form if user clicked Close button (7ms)
    ✓ should close form if user submit the form (139ms)
...

Test Suites: 1 skipped, 91 passed, 91 of 92 total
Tests:       38 skipped, 899 passed, 937 total
Snapshots:   7 passed, 7 total
Time:        14.389s
Ran all test suites.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

hideokamoto added 2 commits

November 18, 2017 20:49


          Add UrlInputButton unit test

fcfd888


          UrlInputButton unit test (change / click / submit event support)

codecov bot commented Nov 18, 2017 •

edited

Loading

Codecov Report

Merging #3550 into master will increase coverage by 0.86%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3550      +/-   ##
==========================================
+ Coverage    34.9%   35.76%   +0.86%     
==========================================
  Files         263      267       +4     
  Lines        6727     6744      +17     
  Branches     1227     1221       -6     
==========================================
+ Hits         2348     2412      +64     
+ Misses       3694     3657      -37     
+ Partials      685      675      -10

Impacted Files	Coverage Δ
blocks/url-input/button.js	`100% <ø> (+100%)`	⬆️
blocks/api/validation.js	`91.46% <0%> (-3.7%)`	⬇️
blocks/library/button/index.js	`9.3% <0%> (ø)`	⬆️
blocks/hooks/index.js	`100% <0%> (ø)`	⬆️
editor/modes/visual-editor/index.js	`0% <0%> (ø)`	⬆️
editor/utils/with-change-detection/index.js	`100% <0%> (ø)`	⬆️
editor/store-defaults.js	`100% <0%> (ø)`	⬆️
editor/header/fixed-toolbar-toggle/index.js	`0% <0%> (ø)`	⬆️
editor/header/ellipsis-menu/index.js	`0% <0%> (ø)`	⬆️
editor/modes/text-editor/index.js	`0% <0%> (ø)`	⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1709207...ebfa9c3. Read the comment docs.

gziolo added the [Type] Automated Testing label

gziolo requested changes

View reviewed changes

Member

gziolo left a comment

@hideokamoto, thanks for starting this PR. Your tests look very good and work perfectly fine. I left a few comments where we can code something differently. Let me know what do you think about those suggestions.

I also think that descriptions put on it blocks could be further improved.

blocks/url-input/test/button.js

+               */
+              import { shallow, mount } from 'enzyme';
+              /**

Member

gziolo Nov 20, 2017

The same section is already defined above and it doesn't contain any import statements. Let's remove this obsolete comment.

blocks/url-input/test/button.js Outdated

+              describe( 'UrlInputButton', () => {
+              	it( 'should has valid class in wrapper tag', () => {
+              		const wrapper = shallow( <UrlInputButton /> );
+              		expect( wrapper.hasClass( 'blocks-url-input__button' ) ).toBeTruthy();

Member

gziolo Nov 20, 2017

When validating boolean values toBe( true ) seems to be a better choice, which performs the comparison using ===. See this thread on StackOverflow where it is greatly explained.

blocks/url-input/test/button.js Outdated

+              		const wrapper = shallow( <UrlInputButton onChange={ onChangeMock } /> );
+              		wrapper.setState( { expanded: true } );
+              		wrapper.find( { value: '' } ).simulate( 'change' );
+              		expect( onChangeMock.mock.calls.length ).toBe( 1 );

Member

gziolo Nov 20, 2017

Jest also offers toHaveBeenCalledTimes which in my opinion reads better:

expect( onChangeMock ).toHaveBeenCalledTimes( 1 );

blocks/url-input/test/button.js Outdated

+              	it( 'should call onChange function at once if value changes at once', () => {
+              		const onChangeMock = jest.fn();
+              		const wrapper = shallow( <UrlInputButton onChange={ onChangeMock } /> );
+              		wrapper.setState( { expanded: true } );

Member

gziolo Nov 20, 2017

I was wondering why this code updates the state of the component externally instead of using the same event as in the previous test. Can you explain?

To make it easier to read we can wrap this method in a function like this:

const startEditing = ( wrapper ) => wrapper.find( 'IconButton.components-toolbar__control' ).simulate( 'click' );

and then use it as follows:

startEditing( wrapper );

blocks/url-input/test/button.js Outdated

+              		const onChangeMock = jest.fn();
+              		const wrapper = shallow( <UrlInputButton onChange={ onChangeMock } /> );
+              		wrapper.setState( { expanded: true } );
+              		wrapper.find( { value: '' } ).simulate( 'change' );

Member

gziolo Nov 20, 2017

Another option is to look for UrlInput instead of an element with an empty value:

wrapper.find( 'UrlInput' ).simulate( 'change' );

Contributor Author

hideokamoto Nov 21, 2017

Thanks !

But I've try this code, our test doesn't work well..

 FAIL  blocks/url-input/test/button.js
  ● UrlInputButton › should call onChange function at once if value changes at once

    Method “simulate” is only meant to be run on a single node. 0 found instead.
      
      at ShallowWrapper.single (node_modules/enzyme/build/ShallowWrapper.js:1614:17)
      at ShallowWrapper.simulate (node_modules/enzyme/build/ShallowWrapper.js:925:21)
      at Object.<anonymous> (blocks/url-input/test/button.js:40:28)
      at process._tickCallback (node.js:438:9)

And I've try this code to debug the test.

const onChangeMock = jest.fn();
const wrapper = shallow( <UrlInputButton onChange={ onChangeMock } /> );
expect(wrapper.debug()).toBe('')

I think UrlInput tag is converted as '_class'.

<div className=\"blocks-url-input__button\">
      <IconButton icon=\"admin-links\" label=\"Edit Link\" onClick={[Function]} className=\"components-toolbar__control\" />
      <form className=\"blocks-format-toolbar__link-modal\" onSubmit={[Function]}>
        <IconButton className=\"blocks-url-input__back\" icon=\"arrow-left-alt\" label=\"Close\" onClick={[Function]} />
        <_class value=\"\" onChange={[Function]} />
        <IconButton icon=\"editor-break\" label=\"Submit\" type=\"submit\" />
      </form>
    </div>

So I'll try to use data-attributes to getUrlInput component.

reference: https://blog.kentcdodds.com/making-your-ui-tests-resilient-to-change-d37a6ee37269

Member

gziolo Nov 21, 2017

<_class value=\"\" onChange={[Function]} /> - Yes, somehow it doesn't have display name set. We can use data-test for the time being. I will look into it later.

gziolo assigned hideokamoto

Contributor Author

hideokamoto commented Nov 20, 2017

Thanks for reviewing my code :)
I'll check it and update my PR in this week.

hideokamoto added 4 commits

November 21, 2017 13:26


          fix test script

96df57e


          update test for pr-#3550

8d47323


          fix lint error

175bf6a


          update url-input button test

3234b52

gziolo approved these changes

View reviewed changes

Member

gziolo commented Nov 21, 2017

@hideokamoto thanks for addressing my comments, much appreciated. I will polish test descriptions and merge.

gziolo added 2 commits

November 21, 2017 08:20


          Testing: Refine test descriptions for UrlInput

8f61a40


          Testing: Fix Eslint issue with trailing spaces

ebfa9c3

Member

gziolo commented Nov 21, 2017

It should be good to go 🚢

gziolo merged commit 8f57c66 into WordPress:master

hideokamoto deleted the add/test/url-input/button branch

November 21, 2017 07:46

aduth reviewed

View reviewed changes

blocks/url-input/button.js

@@ @@ -59,7 +59,7 @@ class UrlInputButton extends Component { @@
               							label={ __( 'Close' ) }
               							onClick={ this.toggle }
               						/>
-              						<UrlInput value={ url || '' } onChange={ onChange } />
+              						<UrlInput value={ url || '' } onChange={ onChange } data-test="UrlInput" />

Member

aduth Jul 16, 2018

We shouldn't be affecting the output of the original component just to satisfy needs of unit testing. We could have achieved the same with wrapper.find( UrlInput )

Member

gziolo Jul 16, 2018

Yes, agreed. It was a really bad idea which would only work if we would be able to provide Babel plugin which removes those data-test attributes ...

Member

aduth Jul 16, 2018

aduth mentioned this pull request

Testing: Remove data-test attribute from UrlInputButton output #7981

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Automated Testing