[RNMobile] Gallery block: Add integration tests related to its manual tests #38571
Conversation
fluiddot
added
the
Mobile App - i.e. Android or iOS
# Conflicts: # packages/block-library/src/gallery/test/index.native.js # test/native/helpers.js
|
Size Change: 0 B Total Size: 1.15 MB ℹ️ View Unchanged
|
test/native/helpers.js
Outdated
| */ | ||
| export async function initializeEditor( props, { component = Editor } = {} ) { | ||
| export async function executeStoreResolvers( fn ) { |
There was a problem hiding this comment.
I like the idea of decoupling a lot of the initialization logic from initializeEditor if it is useful in isolation.
I just still find the initialization logic fairly confusing. I was trying to reason through why we are providing a delegate callback fn as an argument to executeStoreResolvers that appears to be invoked before anything async is awaited, and it reads like it's because we need the Redux Store to implicitly get instantiated when the EditorComponent is rendered so that any nested components dependent on the Store hook into it for dispatching/selecting. I was trying to understand what we consider "resolvers" by looking through the src/redux-store module.
executeStoreResolvers doesn't really appear to "execute" anything. It appears to mostly just mock timers before, and unmock after, the component that indirectly is provided the Redux Store is rendered.
At the very least, would it help to rename executeStoreResolvers/fn/result to something less ambiguous? I'm struggling at the moment to think of names myself.
export async function executeStoreResolvers( onRender ) {
...
const result = onRender();
...
export async function initializeEditor( props, { component = Editor } = {} ) {
const onRender = () => { const screen = render(<EditorComponent .../>); ... return screen; };
...
return mockEnvironmentBeforeAndUnmockAfterRender( onRender );
...
Does this make any sense?
| await notifySucceedState( media[ 0 ] ); | ||
| await notifySucceedState( media[ 1 ] ); | ||
|
|
||
| expect( getEditorHtml() ).toMatchSnapshot(); |
There was a problem hiding this comment.
Preface: Thank you for adding all of these tests for the Gallery Block. ❤️
I'm concerned with the inclusion of snapshot testing as I've only ever seen them be more troublesome than the value the allegedly provide. When a snapshot test fails, it's generally fairly difficult to understand if the snapshot assertion failed for the right or wrong reasons, e.g.,
- the test assertion for the subject under test legitimately failed
- some other nested component implementation detail indirectly changed the snapshot
, especially when you weren't the "original code owner."
In my previous environment, many times this resulted in the snapshots being improperly updated to reflect invalid new assertions that weren't in line with the expectations of the original test assertion. I'm not sure that that would happen here, but it's just given me this "PTSD" of "if we're using toMatchSnapshot we're probably not asserting correctly."
What do you think?
There was a problem hiding this comment.
I totally agree and to be honest, I'm not a big fan of snapshot testing for the same arguments you mentioned. However, I'd like to note that in this case the snapshots are generated from the HTML output of the editor, not the classic React tree, so I don't initially foresee those problems.
In fact, I decided to go this way in order to simplify the tests and make them smaller, when we want to check if the HTML output of the editor is the expected value. To clarify this, let me share an example:
it( 'disables crop images setting (using snapshot testing)', async () => {
// Initialize with a gallery that contains one item
const screen = await initializeWithGalleryBlock( {
numberOfItems: 1,
media,
} );
const { getByText } = screen;
await openBlockSettings( screen );
// Disable crop images setting
fireEvent.press( getByText( 'Crop images' ) );
expect( getEditorHtml() ).toMatchSnapshot();
} );
it( 'disables crop images setting (not using snapshot testing)', async () => {
const expectedHTML = `<!-- wp:gallery {"imageCrop":false,"linkTo":"none"} -->
<figure class="wp-block-gallery has-nested-images columns-default"><!-- wp:image {"id":2000} -->
<figure class="wp-block-image"><img src="https://test-site.files.wordpress.com/local-image-1.jpeg" alt="" class="wp-image-2000"/></figure>
<!-- /wp:image --></figure>
<!-- /wp:gallery -->`;
// Initialize with a gallery that contains one item
const screen = await initializeWithGalleryBlock( {
numberOfItems: 1,
media,
} );
const { getByText } = screen;
await openBlockSettings( screen );
// Disable crop images setting
fireEvent.press( getByText( 'Crop images' ) );
expect( getEditorHtml() ).toBe( expectedHTML );
} );There was a problem hiding this comment.
That does make me feel better about the odds that the snapshot serialization changes inadvertently when child component implementation details change. However, is there a reason that it wouldn't still be possible even if it was simply the serialization of the HTML output of the editor? I was trying to deep dive into the subscribeParentGetHtml implementation that interacts through the bridge with the requestGetHtml event to get a feel for the details. So far all files I've looked into have .native suffixes or are in the native layer so it doesn't appear that the core gutenberg code appears to be involved much, but I'm not confident.
That being said, I don't feel as strongly against using these types of snapshots. Even if it was possible for these to change inadvertently, I don't imagine the serialization of each block type changes fairly often. The most that would likely change would be default metadata in the HTML comments, and that would likely be a valid reason for an assertion to fail.
Thanks for elaborating further. I appreciate it. I am curious as to your thoughts about my question above so I won't resolve the conversation at the moment just so it doesn't get lost, but I intend to resolve the conversation thread.
There was a problem hiding this comment.
However, is there a reason that it wouldn't still be possible even if it was simply the serialization of the HTML output of the editor
Of course, it would be still possible but I foresee similar odds as if we place the HTML expectation within the test code. Eventually, we might have a fixture file to include them, but I think it would be turn out to be quite similar to what we have currently in the snapshot file. So I don't have a strong opinion on whether to include it, to be honest, I'm open to both.
There was a problem hiding this comment.
I think everything looks good to me in general. We may just want to update the name to waitForStoreResolvers, and anything that @geriux may be waiting on. I think I feel comfortable providing an approval after the minor change.
Great, thanks for reviewing it 🙇. I'll try to apply the suggestion as soon as possible. |
Don't feel rushed. 🙂 |
# Conflicts: # test/native/helpers.js
|
Thanks @geriux and @ttahmouch for reviewing the PR, I'd like to let you know that I pushed a last commit in order to rename a helper, as it was mentioned in this comment. Apart from that, the PR is ready for a final review before merging, thanks 🙇 ! |
| * @return {import('@testing-library/react-native').RenderAPI} A Testing Library screen. | ||
| */ | ||
| export async function initializeEditor( props, { component = Editor } = {} ) { | ||
| return waitForStoreResolvers( () => { |
There was a problem hiding this comment.
The usages of waitForStoreResolvers look good to me. Thanks for taking it into consideration. I appreciate it.
This was automated in the following: WordPress/gutenberg#38571
This was automated in the following: WordPress/gutenberg#38571
This was automated in the following: WordPress/gutenberg#38571
This was automated in the following: WordPress/gutenberg#38571
This was automated in the following: WordPress/gutenberg#38571
* docs: Capture Attachment Page known issue * docs: Remove TC010 Columns front-end presentation test Other Columns tests assert the expected mark up. I think it is safe to presume that if the mark up is correct, the front-end website will render correctly in the browser. * docs: Remove TC010 Gallery rearrange items test This was automated in the following: WordPress/gutenberg#38571 * docs: Remove TC012 Gallery Link to test This was automated in the following: WordPress/gutenberg#38571 * docs: Remove TC002 VideoPress caption test This was automated in the following: wordpress-mobile/gutenberg-mobile#5826 * docs: Remove TC001 Color settings segmented controls test Manually or automatically testing animations feels like a low return on effort required. * docs: Remove TC004 Color settings swatch press test Manually or automatically testing animations feels like a low return on effort required. * docs: Remove unused gallery-settings-link-to image
Fixes wordpress-mobile/gutenberg-mobile#4532 and fixes wordpress-mobile/gutenberg-mobile#3670.
Description
Include integration tests for the Gallery block based on the manual tests for that block (reference).
Apart from the new tests, the following additional changes will be incorporated:
block-editorpackage. This was required to test the flows of media management.testIDprop has been added to the media options picker, in order to be able to query it in the tests.Modalcomponent is visible has been applied to the custom matchertoBeVisible.NOTE: This change has required updating the Jest configuration to prevent helpers files to be considered test files.
Testing Instructions
npm run native testand observe that all tests succeed.Unit Tests / Mobilesucceeds.Screenshots
N/A
Types of changes
Add integration tests
Checklist:
*.native.jsfiles for terms that need renaming or removal).