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

Add full coverage for REQUEST_POST_UPDATE_SUCCESS effect. #3820

Merged
merged 3 commits into from
Dec 15, 2017
+100 −23
Merged

Add full coverage for REQUEST_POST_UPDATE_SUCCESS effect. #3820

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a329322
Add full coverage for REQUEST_POST_UPDATE_SUCCESS effect.
BE-Webdesign Dec 5, 2017
e6796f9
Updating effects to make use of matchers instead of explicit matching.
BE-Webdesign Dec 14, 2017
fb96084
Tests: Refactor tests for REQUEST_POST_UPDATE_SUCCESS effects
gziolo Dec 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 117 additions & 4 deletions editor/test/effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

window.history.replaceState = jest.spyOn( window.history, 'replaceState' );
Expand All @@ -296,11 +298,10 @@ describe( 'effects', () => {
window.history.replaceState.mockRestore();
} );

const handler = effects.REQUEST_POST_UPDATE_SUCCESS;
const dispatch = jest.fn();
const store = { getState: () => {}, dispatch };

it( 'should dispatch meta box updates on success for dirty meta boxes.', () => {
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.

const store = { getState: () => {}, dispatch };

selectors.getDirtyMetaBoxes.mockReturnValue( [ 'normal', 'side' ] );

const post = {
Expand All @@ -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

const dispatch = jest.fn();
const store = { getState: () => {}, dispatch };

selectors.getDirtyMetaBoxes.mockReturnValue( [ 'normal', 'side' ] );

const post = {
id: 1,
title: {
raw: 'A History of Pork',
},
content: {
raw: '',
},
status: 'publish',
};

const previousPost = {
...post,
status: 'draft',
};

handler( { post, previousPost }, store );

expect( dispatch ).toHaveBeenCalledTimes( 2 );
expect( dispatch ).toHaveBeenCalledWith( expect.objectContaining( {
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.

id: 'SAVE_POST_NOTICE_ID',
isDismissible: true,
status: 'success',
},
type: 'CREATE_NOTICE',
} ) );
} );

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

:)

const dispatch = jest.fn();
const store = { getState: () => {}, dispatch };

selectors.getDirtyMetaBoxes.mockReturnValue( [ 'normal', 'side' ] );

const post = {
id: 1,
title: {
raw: 'A History of Pork',
},
content: {
raw: '',
},
status: 'draft',
};

const previousPost = {
...post,
status: 'publish',
};

handler( { post, previousPost }, store );

expect( dispatch ).toHaveBeenCalledTimes( 2 );
expect( dispatch ).toHaveBeenCalledWith( expect.objectContaining( {
notice: {
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.

</p>,
id: 'SAVE_POST_NOTICE_ID',
isDismissible: true,
status: 'success',
},
type: 'CREATE_NOTICE',
} ) );
} );

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.

const dispatch = jest.fn();
const store = { getState: () => {}, dispatch };

selectors.getDirtyMetaBoxes.mockReturnValue( [ 'normal', 'side' ] );

const post = {
id: 1,
title: {
raw: 'A History of Pork',
},
content: {
raw: '',
},
status: 'publish',
};

const previousPost = {
...post,
status: 'publish',
};

handler( { post, previousPost }, store );

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.

id: 'SAVE_POST_NOTICE_ID',
isDismissible: true,
status: 'success',
},
type: 'CREATE_NOTICE',
} ) );
} );
} );

describe( '.SETUP_EDITOR', () => {
Expand Down