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

Add tests for the UrlInputButton component. #3550

Merged
merged 8 commits into from Nov 21, 2017

Conversation

Projects
None yet
3 participants
@hideokamoto
Contributor

hideokamoto commented Nov 18, 2017

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.
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 18, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

codecov bot commented Nov 18, 2017

Codecov Report

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

Impacted file tree graph

@@            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

@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.

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

This comment has been minimized.

@gziolo

gziolo Nov 20, 2017

Member

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

@gziolo

gziolo Nov 20, 2017

Member

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

Show outdated Hide outdated blocks/url-input/test/button.js Outdated
Show outdated Hide outdated blocks/url-input/test/button.js Outdated
Show outdated Hide outdated blocks/url-input/test/button.js Outdated
Show outdated Hide outdated blocks/url-input/test/button.js Outdated
@hideokamoto

This comment has been minimized.

Show comment
Hide comment
@hideokamoto

hideokamoto Nov 20, 2017

Contributor

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

Contributor

hideokamoto commented Nov 20, 2017

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

@gziolo

gziolo approved these changes Nov 21, 2017

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 21, 2017

Member

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

Member

gziolo commented Nov 21, 2017

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

gziolo added some commits Nov 21, 2017

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 21, 2017

Member

It should be good to go 🚢

Member

gziolo commented Nov 21, 2017

It should be good to go 🚢

@gziolo gziolo merged commit 8f57c66 into WordPress:master Nov 21, 2017

2 checks passed

codecov/project 35.76% (+0.86%) compared to 1709207
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hideokamoto hideokamoto deleted the hideokamoto:add/test/url-input/button branch Nov 21, 2017

@@ -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" />

This comment has been minimized.

@aduth

aduth Jul 16, 2018

Member

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 )

@aduth

aduth Jul 16, 2018

Member

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 )

This comment has been minimized.

@gziolo

gziolo Jul 16, 2018

Member

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 ...

@gziolo

gziolo Jul 16, 2018

Member

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 ...

This comment has been minimized.

@aduth

aduth Jul 16, 2018

Member

See #7981

@aduth

aduth Jul 16, 2018

Member

See #7981

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