-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 Unit test for /editor/actions.js
#3787
Conversation
editor/test/actions.js
Outdated
convertBlockToStatic, | ||
convertBlockToReusable, | ||
} from '../actions'; | ||
import * as actions from '../actions'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think using wildcard makes it easier as you need to prefix all method with actions.
. It's also a common pattern used to import everything explicitly. It isn't as important in the tests context but can make a difference when bundling code for the browser (it would prevent using tree-shaking feature).
editor/test/actions.js
Outdated
} ); | ||
describe( 'removeNotice', () => { | ||
it( 'should return REMOVE_NOTICE actions', () => { | ||
const id = 'id'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id
is used only once. It should be either inlined or reused in the toEqual
call. The same applies to location
and feature
in the 2 next tests.
editor/test/actions.js
Outdated
const content = <p>element</p>; | ||
it( 'should return CREATE_NOTICE action when options is empty', () => { | ||
const result = actions.createNotice( status, content ); | ||
expect( result.type ).toEqual( 'CREATE_NOTICE' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toMatchObject would be a perfect fit here and for the similar tests.
expect( result).toMatchObject( {
type: 'CREATE_NOTICE',
notice: {
status, // it's defined above
content, // it's also defined above
isDismissible: true,
id: expect.any( Number ),
},
} );
editor/test/actions.js
Outdated
} ); | ||
describe( 'replaceBlock', () => { | ||
it( 'should return the REPLACE_BLOCKS action', () => { | ||
const blocks = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a single block so the name should be block
.
Awesome work adding all those missing tests. I left some comments where we could consider changes. In general most of the tests define const at the beginning of the test which is used only once. It could be reused when composing expected result. Alternatively, they could be inlined. I prefer the former, as we are only interested whether the output shape is properly created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as needs author's feedback before proceeding. Let me know what do you think about my comments left.
Thanks for feedback ! Please wait for a while 👍 |
Hi @gziolo . Could you review again my PR ? |
All actions are covered with tests with this PR. Awesome work 👍 |
Thanks for addressing my comments and your contribution :) |
Description
Related to progress on the issue -> Unit Testing Components #641
Added some unit test scripts for actions.
How Has This Been Tested?
Run unit test command.
Checklist: