block-directory: migrate store actions to thunks #35031
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsnajdr
added
the
[Feature] Block Directory
jsnajdr
force-pushed
the
migrate/block-directory-thunks
branch
from
13e348c
to
f5ced00
Compare
|
Size Change: -8 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
adamziel
reviewed
Sep 22, 2021
| return false; | ||
| } | ||
| return state.downloadableBlocks[ filterValue ].isRequesting; | ||
| return state.downloadableBlocks[ filterValue ]?.isRequesting ?? false; |
adamziel
reviewed
Sep 22, 2021
| const registeredBlock = registry | ||
| .select( blocksStore ) | ||
| .getBlockType( 'block/block' ); | ||
| expect( registeredBlock ).toBeTruthy(); |
There was a problem hiding this comment.
I understand why you did that, and at the same time testing an action by calling selectors feels off to me. A failed test could indicate a problem with any function called here. What's your take on mocking dispatch and registry calls?
There was a problem hiding this comment.
I have the same question for other tests; let's keep the discussion contained here.
There was a problem hiding this comment.
What's your take on mocking dispatch and registry calls?
I think that not a good way to unit test things and I strongly prefer to test the entire store as one unit. The actions, reducers, resolvers and selectors are not very interesting as individual units -- it's their combination into a store where the interesting and test-worthy behavior emerges.
When mocking dispatch and everything else, I'm basically testing only that the action function calls other functions, but I already know that from reading the source code. I'd rather test what the function returns and/or what effects does the call have.
There was a problem hiding this comment.
I see, what we're really discussing is what unit is being tested here. If I think about the store, not a specific action, it makes much more sense. And we have unit tests for these selectors, so should be easy to identify where did the failure originate.
There was a problem hiding this comment.
Testing a selector as a standalone unit makes sense, because it converts a state object to a return value and I just need to supply the state object. Similarly, reducer takes a state and an action and produces a new state. That's also self-contained even without a store.
But a complex action thunk that performs multiple operations on several registered stores? To really test what's the observable result of that action, I need a realistic registry with the stores.
To show an analogy, consider a function that converts an array into another array. I can test it by passing some testing array as an argument and checking if the returned array is as expected. That's the common approach. Or I could test it by mocking the Array class and its methods, and then checking that the tested function called map, push, slice and sort in the expected order and with expected arguments. That would surprise most people and yet that's exactly how we're used to test @wordpress/data actions.
Or a function that uses the DOM API (createElement, insertBefore, append, ...) to add some HTML markup to the document. One way to test is to use jsdom and then check if the created markup has the structure we want (like <h1><b>hello</b> world</h1>). Another way is to mock the DOM APIs and check if the mocks were called in the expected order and the expected number of times. Again, the first approach is usually the preferred one.
There was a problem hiding this comment.
You are full of great insights @jsnajdr, thank you for elaborating! I'll borrow some of that code to test the other stores
adamziel
reviewed
Sep 22, 2021
adamziel
reviewed
Sep 22, 2021
| const registry = createRegistryWithStores(); | ||
|
|
||
| apiFetch.mockImplementation( async ( { url, method } ) => { | ||
| switch ( url ) { |
…sets doesn't have the param
adamziel
force-pushed
the
migrate/block-directory-thunks
branch
from
f5ced00
to
f2a0b6c
Compare
adamziel
approved these changes
Sep 22, 2021
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR migrates the
block-directorydata store to thunks, namely theinstallBlockTypeanduninstallBlockTypeactions. And removes dependency on@wordpress/data-controlsand on any other controls for that matter.The first three commits do some preparatory fixing and cleanups:
optional?.chaining?.[ 'and' ] ?? 'coalescing'in selectors to make them nicerassets: [ ... ]field on the block search results and theloadAssets( assets )doesn't really have/use theassetsparam so I'm removing that.loadAssetsfetches the HTML document after a plugin was installed, looks forscripttags that are new, and loads them into the current document.blockobject more close to what a real REST endpoint for block directory search would return, namely there areid(plugin id),name(block id) andtitle(block name for humans) fields.And then there's the real migration in the commit number four.
I completely reworked the unit test for actions. Instead of checking the objects produced by the generator, they set up a real registry with real stores, mock the external modules like
apiFetchorloadAssets, and then dispatch real actions to the registry and check how that affects the state. That's still a self-contained unit test, but way closer to how the code is used in the real editor.How to test:
In the block inserter, search for a block you don't have installed (I use a
mathmlsearch term and install the MathML block for testing) and verify you can install it from block directory. Or when the plugin is installed and merely deactivated, that you can activate it from the block directory UI.Then, if you don't use that new block in your document, and do a save, that the block gets automatically uninstalled.