[RNMobile] Add API fetch test helper #50391
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
fluiddot
added
the
Mobile App - i.e. Android or iOS
fluiddot
force-pushed
the
rnmobile/add/api-fetch-test-helper
branch
from
1e385f7 to
8379426
Compare
|
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
SiobhyB
approved these changes
May 8, 2023
| ); | ||
| } | ||
| ); | ||
| return matchedResponse?.response; |
There was a problem hiding this comment.
Would there be a benefit to logging an error here if there is no matchedResponse?
if ( ! matchedResponse ) {
throw new Error( `No response for path: ${ path }` );
}
return matchedResponse.response;
There was a problem hiding this comment.
Actually, I originally added this condition but noticed the editor makes a set of requests always upon its initialization. This made me think that probably makes more sense to delegate checking what requests should be made or shouldn't to the test logic, instead here. In any case, we could add an option to make this function more restrictive and fail the test if an unexpected request is made. Maybe it's useful in some cases 🤔 .
For reference, the following requests are made when the editor is initialized:
/wp/v2/settings/wp/v2/media/wp/v2/pages/wp/v2/block-patterns/patterns/wp/v2/block-patterns/categories/wp/v2/posts/post-id-<POST_GUID>
There was a problem hiding this comment.
Thank you for sharing the logic behind that decision, that makes sense to me.
In any case, we could add an option to make this function more restrictive and fail the test if an unexpected request is made. Maybe it's useful in some cases 🤔 .
I trust your judgement, but have a sense that the value would be relatively low. The current code looks good to me :) thanks again for expanding on the logic here.
There was a problem hiding this comment.
I trust your judgement, but have a sense that the value would be relatively low. The current code looks good to me :) thanks again for expanding on the logic here.
Ok, I'll merge the PR as-is, thanks!
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.
What?
Adds a test helper to allow mocking API fetch requests in tests.
Why?
This helper will simplify the test logic for cases blocks/components make API requests.
How?
A new helper has been added that accepts an array of objects with the following parameters:
request: This object contains the parameters that we'll use to match requests. The parameters are the same as we pass to theapiFetchfunction.response: The result we want to return to requests that match the parameters set above.Additionally, some tests of the Image block have been updated to use this helper. This block fetches media data from the API when is mounted, so it was a good example to use the helper.
Testing Instructions
npm run native test -- image/test/edit.nativeUnit Tests / MobileCI jobs pass.Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A