Mobile unit tests: remove custom waitFor implementation #46735
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
|
Size Change: -26 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
jsnajdr
force-pushed
the
remove/mobile-custom-waitfor
branch
from
a9c4199
to
2083529
Compare
|
One unit test is failing, auto-pasting of link URL in Embed block, and it's a bug in |
|
Fixing the This PR is on hold until we can upgrade to a fixed version of the library. |
jsnajdr
force-pushed
the
remove/mobile-custom-waitfor
branch
from
2083529
to
cecac95
Compare
|
Flaky tests detected in b880c60. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5335657048
|
jsnajdr
force-pushed
the
remove/mobile-custom-waitfor
branch
from
cecac95
to
0104956
Compare
jsnajdr
force-pushed
the
remove/mobile-custom-waitfor
branch
2 times, most recently
from
a1e496f
to
1278210
Compare
jsnajdr
requested review from
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
I had to change one more thing to make all tests green. Since v12.0.0, RNTL by default excludes elements hidden from accessibility from query results. And we need to find them explicitly with a Most of the fixes were in usages of the To test inaccessible parts of the UI, we need to declare explicitly we want to look at inaccessible elements. This doesn't mean that the UI wouldn't be accessible -- it just exposes two interfaces, accessible and "visual", and we want to test the visual one, too. The only shortcoming of |
jsnajdr
force-pushed
the
remove/mobile-custom-waitfor
branch
from
66f9cf5
to
ff28fe5
Compare
dcalhoun
approved these changes
Jun 16, 2023
There was a problem hiding this comment.
This is fantastic! 💯
I do not believe I can express how much this particular work means to me personally. This is one of those "thorns" that I have been unable to set aside time to address. Thank you! 🙇🏻
I left a couple of comments, but I do not consider any of them blocking. So, with that, I approved these changes. I do not want to block merging or open up the potential for merge conflicts by unnecessarily postponing. 🚀
packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js
Show resolved
Hide resolved
| fireEvent.press( screen.getByText( 'None' ) ); | ||
| fireEvent.press( screen.getByText( 'Media File' ) ); |
There was a problem hiding this comment.
IIRC, this test's original intention was to verify the following flow: None → Media File → Custom URL → Media File. This was done because a scenario where not all properties were correctly updated during the flow.
Because of that, it may make most sense to reinstate Media File via two presses on that text. Once to select Media File, another to navigate back to the list of options. WDYT?
Video of Flow
RPReplay_Final1686932123.MP4
There was a problem hiding this comment.
I'll have a second look at this test in the next few days.
I merged this PR now because I was afraid that it would get stalled again for a long time if I wanted to make everything perfect before merging.
Comment on lines
+175
to
+178
| // wait until inserter on the newly created indented block is enabled | ||
| // this is slightly delayed (by updating block list settings) and would | ||
| // trigger an "update not wrapped in act()" warning if not explicitly awaited. | ||
| screen.findByRole( 'button', { name: 'Add block', disabled: false } ); |
There was a problem hiding this comment.
This is a fantastic note and approach. Thank you for sharing this.
@fluiddot it is likely beneficial to document this and leverage it in other scenarios.
| @@ -251,7 +251,7 @@ describe( 'Paragraph block', () => { | |||
| 'wordpress.org' | |||
| ); | |||
| fireEvent.changeText( | |||
| screen.getByPlaceholderText( 'Add link text' ), | |||
| screen.getByPlaceholderText( 'Add link text', { hidden: true } ), | |||
There was a problem hiding this comment.
Noting this relates to #51303 (comment), which refactors this test to interact with the element prior to it becoming inaccessible. I think it is fine for this PR to leave the hidden addition for now, but we should prefer avoiding its addition long term.
jsnajdr
force-pushed
the
remove/mobile-custom-waitfor
branch
from
ff28fe5
to
b880c60
Compare
|
So cool to see this land 👏 |
|
Just wanted to drop by to say thank you so much @jsnajdr for working on this, awesome job 🙇 . I'm glad we managed to remove the custom implementation of |
|
Thanks @fluiddot for your kind words 👍 If you ever need a helping hand with low level stuff like this, e.g., when upgrading React Native, please let me know. |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
) * useNestedSettingsUpdate: don't rerender if blockListSettings value changes * Mobile unit tests: remove custom waitFor implementation * Upgrade @testing-library/react-native to 12.1.2, fixes effect flushing bugs * Remove references to legacy screen.container, prefer snapshots * Spacer block: use waitForModalVisible * Image test: remove unneeded workarounds * Queries exclude elements hidden from accessibility, find them explicitly
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.
The mobile tests, due to some past bugs in
@testing-library/react-native, had their own implementation ofwaitForand avoided using thefindBy*query helpers, in favor of the less-recommendedwaitFor(() => getBy*)form.This PR removes the custom
waitFor. It was there because of bugs in@testing-library/react-native(some fixed by myself in 12.0.1 but there are others) that needed to be worked around. That's why this PR also upgrades the testing library to its latest version 12.1.2.In addition to modifying tests, this PR also modifies the
useNestedSettingsUpdatefunction. It used to retrieve the current value ofblockListSettings, and to shallowly compare it withnewSettings. But retrieving theblockListSettingswithuseSelectmeans that the component is rerendered after changing the settings, although it doesn't use them directly. This extra rerender was causingact()warnings. My patch is removing the retrieval and comparison, and just dispatches theupdateBlockListSettingsdirectly, even if thenewSettingsare the same. The state reducer itself will check, when handlingUPDATE_BLOCK_LIST_SETTINGS, whether there are any real changes.