-
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
Thunkify reusable-blocks store #35217
Conversation
Size Change: -23 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
] ); | ||
|
||
return registry; | ||
} | ||
|
||
describe( '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.
So these seem more like integration tests now that the entire store is being tested. I quite like it, it feels more useful to test stores as a standalone module.
Is it worth still keeping separate unit tests for selectors and reducers given they are also tested here?
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.
Is it worth still keeping separate unit tests for selectors and reducers given they are also tested here?
I think it is worth it, it creates an easy path of zeroing-in on a problem: imagine the actions-related tests fail, but the selectors-related tests don't. Now we know the selectors are fine.
expect( updatedBlocks ).toHaveLength( 1 ); | ||
|
||
// Delete random clientId before matching with snapshot | ||
delete updatedBlocks[ 0 ].clientId; |
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 had a similar issue in another test and decided to do this:
updatedBlocks[ 0 ].clientId = `[${ typeof updatedBlocks[ 0 ].clientId }]`
At least the test can assert the type of the property is correct. Not something required, but just thought I'd share.
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 like that idea! I ended up merging this PR as is, but I may follow up with that change. For sure it's something I'll use in the future, thank you!
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.
Looks great, nice job!
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.
Works as advertised 👍
This PR migrates the reusable-blocks data store to thunks.
I reworked the unit tests to check for outcomes instead of the specific flow we tested for inside of the generator.
How to test:
Open the post editor, create a reusable block, reload the page, convert back to static, create another one, delete it, and confirm everything worked as expected.