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.

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 full coverage for REQUEST_POST_UPDATE_SUCCESS effect. #3820

Merged
merged 3 commits into from
Dec 15, 2017
Merged

Add full coverage for REQUEST_POST_UPDATE_SUCCESS effect. #3820

merged 3 commits into from
Dec 15, 2017

Conversation

BE-Webdesign
Copy link
Contributor

Adds test cases to fully cover the REQUEST_POST_UPDATE_SUCCESS effect.

Checklist:

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

@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 Dec 5, 2017
aduth
aduth approved these changes Dec 11, 2017
View reviewed changes
editor/test/effects.js Outdated
it( 'should dispatch meta box updates on success for dirty meta boxes.', () => {
const handler = effects.REQUEST_POST_UPDATE_SUCCESS;
const dispatch = jest.fn();
Copy link
Member

Choose a reason for hiding this comment

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

We might have been able to share these variable references, since it appears the only thing we needed to do was ensure that dispatch calls were reset between tests when asserting call count.

https://facebook.github.io/jest/docs/en/mock-function-api.html#mockfnmockclear

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 changed this, but I guess I never did will patch now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow this broke everything.

editor/test/effects.js Outdated
handler( { post, previousPost }, store );

expect( dispatch ).toHaveBeenCalledTimes( 2 );
expect( dispatch.mock.calls[ 0 ][ 0 ] ).toEqual( {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: A bit fragile to assume the order in which the notice dispatch is called.

editor/test/effects.js Outdated
expect( dispatch ).toHaveBeenCalledTimes( 2 );
expect( dispatch.mock.calls[ 0 ][ 0 ] ).toEqual( {
notice: {
content: <p><span>Post published!</span> <a href={ undefined }>View post</a></p>, // eslint-disable-line jsx-a11y/anchor-is-valid
Copy link
Member

Choose a reason for hiding this comment

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

Could we just leave off the href prop altogether, or are you trying to be explicit here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to be specific, we could use snapshots instead.

editor/test/effects.js Outdated
content: <p>
<span>Post reverted to draft.</span>
{ ' ' }
{ false && <a href={ post.link }>{ 'View post' }</a> }
Copy link
Member

Choose a reason for hiding this comment

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

Is this line necessary? Or again trying to be explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, we could use snapshots instead.

@BE-Webdesign
Copy link
Contributor Author

Going to refactor these to be better.

@BE-Webdesign BE-Webdesign force-pushed the add/test/editor/effects/request-post-update-success branch from 70e3eaf to e6796f9 Compare December 14, 2017 20:59
@BE-Webdesign
Copy link
Contributor Author

@aduth, let me know if this looks better.

gziolo
gziolo reviewed Dec 15, 2017
View reviewed changes
editor/test/effects.js Outdated
content: <p>
<span>Post reverted to draft.</span>
{ ' ' }
{ false && <a href={ post.link }>{ 'View post' }</a> }
Copy link
Member

Choose a reason for hiding this comment

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

I think it's confusing, as it will render false anyway. Let's remove the link part.

gziolo
gziolo reviewed Dec 15, 2017
View reviewed changes
editor/test/effects.js Outdated
@@ -319,6 +320,118 @@ describe( 'effects', () => {
expect( dispatch ).toHaveBeenCalledTimes( 1 );
expect( dispatch ).toHaveBeenCalledWith( requestMetaBoxUpdates( [ 'normal', 'side' ] ) );
} );

it( 'should dispatch notices when reverting a published post to a draft.', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This test should be named:

should dispatch notices when publishing or scheduling a post

gziolo
gziolo reviewed Dec 15, 2017
View reviewed changes
editor/test/effects.js Outdated
} ) );
} );

it( 'should dispatch notices when publishing or scheduling a post.', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This test should be named as the previous one is at the moment:

should dispatch notices when reverting a published post to a draft

:)

gziolo
gziolo reviewed Dec 15, 2017
View reviewed changes
editor/test/effects.js Outdated
@@ -281,6 +281,8 @@ describe( 'effects', () => {
} );

describe( '.REQUEST_POST_UPDATE_SUCCESS', () => {
const handler = effects.REQUEST_POST_UPDATE_SUCCESS;

beforeAll( () => {
selectors.getDirtyMetaBoxes = jest.spyOn( selectors, 'getDirtyMetaBoxes' );
Copy link
Member

Choose a reason for hiding this comment

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

My IDE complains that selectors.getDirtyMetaBoxes is a constant and shouldn't be overridden.

gziolo
gziolo reviewed Dec 15, 2017
View reviewed changes
editor/test/effects.js Outdated
} ) );
} );

it( 'should dispatch notices when just updating a published post again.', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Just noting, that we don't use a period at the end of the test name in other places.

gziolo
gziolo reviewed Dec 15, 2017
View reviewed changes
editor/test/effects.js Outdated
expect( dispatch ).toHaveBeenCalledTimes( 2 );
expect( dispatch ).toHaveBeenCalledWith( expect.objectContaining( {
notice: {
content: <p><span>Post updated!</span>{ ' ' }<a href={ undefined }>{ 'View post' }</a></p>, // eslint-disable-line jsx-a11y/anchor-is-valid
Copy link
Member

Choose a reason for hiding this comment

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

We can omit href if it set to undefined.

@gziolo gziolo force-pushed the add/test/editor/effects/request-post-update-success branch from c8f3116 to fb96084 Compare December 15, 2017 08:34
gziolo
gziolo approved these changes Dec 15, 2017
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I addressed my own comments. It is ready to merge. Thanks @BE-Webdesign for adding all those tests 👍

@gziolo gziolo merged commit 7f346ea into master Dec 15, 2017
@gziolo gziolo deleted the add/test/editor/effects/request-post-update-success branch December 15, 2017 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth approved these changes

@gziolo gziolo gziolo approved these changes

Assignees
No one assigned
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BE-Webdesign @gziolo @aduth