[RNMobile] Upgrade React Native 0.71.11
#51303
Conversation
fluiddot
added
the
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.44 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 5040e6a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5674693795
|
`Linking.removeEventListener` has been removed in RN `0.71`. The library is mocked by default but doesn't return the `remove` function when calling `addEventListener`.
fluiddot
force-pushed
the
rnmobile/upgrade/react-native-0.71.8
branch
from
9cad3d7
to
ca395c2
Compare
| @@ -437,7 +437,7 @@ describe( 'BlockDraggable', () => { | |||
| // activate the gesture. Since this not available in tests, the library | |||
| // displays a warning message. | |||
| expect( console ).toHaveWarnedWith( | |||
| '[react-native-gesture-handler] You have to use react-native-reanimated in order to control the state of the gesture.' | |||
| '[Reanimated] You can not use setGestureState in non-worklet function.' | |||
There was a problem hiding this comment.
The previous warning is no longer shown when running the tests, seems that now a different one is displayed instead.
| "busy": undefined, | ||
| "checked": undefined, | ||
| "disabled": false, | ||
| "expanded": undefined, | ||
| "selected": undefined, | ||
| } | ||
| } | ||
| accessibilityValue={ | ||
| { | ||
| "max": undefined, | ||
| "min": undefined, | ||
| "now": undefined, | ||
| "text": undefined, |
There was a problem hiding this comment.
These new a11y values have been added in RN 0.71 (reference).
| @@ -44,6 +44,7 @@ describe( 'MediaUpload component', () => { | |||
| const wrapper = render( | |||
| <MediaUpload | |||
| allowedTypes={ [ mediaType ] } | |||
| onSelectURL={ jest.fn() } | |||
There was a problem hiding this comment.
This fixes a failure I encountered when running the tests. The error was related to missing the option "Insert from URL" on the Image and Audio block.
After reviewing the logic of MediaUpload component, seems the error is legit, as the URL insertion option is only available when passing the onSelectURL prop. Now I'm curious about how these tests passed before 🤔 .
| // Mock debounce to prevent potentially belated state updates. | ||
| jest.mock( '@wordpress/compose/src/utils/debounce', () => ( { | ||
| debounce: ( fn ) => { | ||
| fn.cancel = jest.fn(); | ||
| return fn; | ||
| }, | ||
| } ) ); |
There was a problem hiding this comment.
The link suggestions are fetched using debounce. Since we are not testing the suggestions in this test suites, I disabled debounce to avoid belated state updates.
| // Mock link suggestions that are fetched by the link picker | ||
| // when typing a search query. | ||
| jest.mock( '@wordpress/core-data/src/fetch', () => ( { | ||
| __experimentalFetchLinkSuggestions: jest.fn().mockResolvedValue( [ {} ] ), | ||
| } ) ); |
There was a problem hiding this comment.
I noticed that the Link picker component is making requests to fetch the link suggestions. This was causing a failure in the tests because, although apiFetch is mocked, it's not returning a promise. This logic is not being tested in these test suites, so I simply mock it.
gutenberg/test/native/setup.js
Lines 73 to 78 in b52d61e
| <div class="wp-block-button"> | ||
| <a class="wp-block-button__link" style="border-radius:5px">Link</a> | ||
| </div> | ||
| <div class="wp-block-button"><a class="wp-block-button__link wp-element-button" style="border-radius:5px">Link</a></div> |
There was a problem hiding this comment.
This HTML has been updated as its format was outdated.
There was a problem hiding this comment.
I suppose this is a good example of the benefit of leveraging UI instead of initial HTML. I wonder if there is are benefits to relying upon initial HTML. Test speed possibly? I'm unsure of if the impact is negligible.
There was a problem hiding this comment.
I think the cases where we use initial HTML are mainly driven to start the editor in a specific state, similar to opening a post with content. This has the downside of using fixed versions of the block's HTML code, which as in this case, it would eventually lead to being outdated.
It's likely that adding the block and setting the content is slower than using the initial HTML. I haven't reviewed thoroughly these test cases to determine if could use a different approach, my main goal in this PR was to simply fix the issues to unblock the RN upgrade. Nevertheless, we could improve this in another PR if we feel the need to.
There was a problem hiding this comment.
I haven't reviewed thoroughly these test cases to determine if could use a different approach, my main goal in this PR was to simply fix the issues to unblock the RN upgrade. Nevertheless, we could improve this in another PR if we feel the need to.
Definitely. No intention from my side to modify the testing approach in this PR. Merely shared the thought for general consideration.
There was a problem hiding this comment.
This patch is no longer necessary as @react-navigation/native has been updated to 6.x (reference).
There was a problem hiding this comment.
This patch replaces the one made for the previous version of react-devtools-core (https://github.com/WordPress/gutenberg/pull/51303/files#diff-efdeee0f12bb2896ff685c25c7b899da1f11686b6d333de8c33265db624b870e).
| // Run all timers, in case any performs a state updates. | ||
| // Column block example: https://t.ly/NjTs | ||
| act( () => jest.runOnlyPendingTimers() ); |
There was a problem hiding this comment.
I am hesitant to await this nuanced UI logic in a test helper. This feels like something that should be accounted for within the Columns test file.
I am unaware of a specific example right now, but I could see abstracting the timers to this top-level helper confusing future test writers. E.g. if my subject block transitions from one state to another over a period of time, i.e. with setTimeout, this global helper would prevent me from asserting (or simply understanding and recognizing) that transition.
Are there act examples outside of the Column block of which you are aware? Does it make sense to relocate this to the block tests that need it?
There was a problem hiding this comment.
I haven't checked if other blocks use timers to update their local state when created. I agree that, if this is only needed for a specific block, we could execute in the associated test files instead of in a generic way.
My purpose with this approach was to simplify the block insertion, as I feel that it won't be very common to assert potential state updates that happen during the insertion. I assumed that all of them acted as part of the addBlock helper. However, in case we need to, we could run the timers conditionally via a configuration parameter.
I'll check if this is needed in more blocks, and depending on that, we could decide if we rather run the timers generally here or in each test 👍 .
There was a problem hiding this comment.
Your expressed purpose makes a lot of sense. I recognize that our testing environment needs to find a balance between convenience and comprehensibility.
To be clear, I am not opposed to inserting this global timer into the addBlock helper. I merely wanted to note its potential for breeding confusion due to the obscure nature of this timers run. The inline comments will likely help to mitigate that, provided the test author reads the source of addBlock when using it.
Please feel free to leave this implementation as-is. A configuration parameter could arguably be unnecessary complexity.
| // Let potential queued microtasks (like Promises) to be executed. | ||
| // Inner blocks example: https://t.ly/b95nA | ||
| await act( async () => {} ); |
There was a problem hiding this comment.
From reviewing the example inner block state changes, it makes sense to abstract this microtask flush, from my perspective.
The changing state is already abstract, not incredibly block-specific, and does not always result in changes that are queryable in the UI.
| Setting | Type | Queryable Result |
|---|---|---|
allowedBlocks |
WPBlockType[] |
Seemingly unavailable. |
orientation |
"vertical" | "horizontal" |
Block mover arrows. |
prioritizedInserterBlocks |
WPBlockType[] |
Seemingly unavailable |
templateLock |
boolean |
Lock icon? |
packages/components/src/mobile/link-settings/test/edit.native.js
Outdated
Show resolved
Hide resolved
| 'wordpress.org' | ||
| ); | ||
| fireEvent.press( screen.getByLabelText( 'Link' ) ); | ||
|
|
||
| fireEvent.changeText( | ||
| screen.getByPlaceholderText( 'Add link text' ), |
There was a problem hiding this comment.
Was populating the text via the link URL causing issue or done merely for clear presentation?
| <div class="wp-block-button"> | ||
| <a class="wp-block-button__link" style="border-radius:5px">Link</a> | ||
| </div> | ||
| <div class="wp-block-button"><a class="wp-block-button__link wp-element-button" style="border-radius:5px">Link</a></div> |
There was a problem hiding this comment.
I suppose this is a good example of the benefit of leveraging UI instead of initial HTML. I wonder if there is are benefits to relying upon initial HTML. Test speed possibly? I'm unsure of if the impact is negligible.
It also upgrades `metro-react-native-babel` dependencies following the upgrade helper.
We only need to mock the return the value, hence we don't need to mock the entire library.
|
Heads up that I upgraded React Native to a new patch version ( |
fluiddot
changed the title
0.71.80.71.11
# Conflicts: # packages/block-library/src/audio/test/__snapshots__/edit.native.js.snap # packages/block-library/src/file/test/__snapshots__/edit.native.js.snap
The `--no-jetifier` option no longer appears to be supported and results in an error when attempting to build the Android demo app. Ref: wordpress-mobile/gutenberg-mobile#5881 (comment)
|
Seems that a couple of the End-to-End Tests / Playwright CI jobs are failing consistently (I retried them three times). I don't think these changes are breaking the web version, so I'm going to update with |
Seems it keeps failing, I'll take a look and run them locally in case any of the changes of the PR introduced a regression. |
In 4482b9d we had a conflict in `package-lock.json` that was solved using the changes from this branch. However, seems that something went wrong and that although the editor has no issues, some e2e tests are failing due to this. This has been solved by using the latest version of `package-lock.json` file from `trunk` and updating it with the package updates required in the React Native upgrade.
This issue should be addressed via c6b1311 (further information can be found in the commit message). From this change, we had also updated a patch and the |
|
@fluiddot I see you experienced the new PR Label enforcer 😅 . I see it's frequent that mobile-app-related PRs don't have any other PRs. Should I add |
Yeah, it's a nice addition 🎊.
Yes, it's not very common although it might be the case when working on a feature.
@priethor To be honest, I think we can benefit from using the type label. This way we can add more context to the PRs 🙂. For now, I'd avoid adding the Mobile label. Thanks 🙇 ! |
|
@fluiddot To clarify, you can (and probably should?) still add the Mobile label! The new pre-merge check ensures all PRs are of type enhancement, bugfix, etc. So nothing prevents adding a type like |
Oh sorry, I didn't want to imply that we won't use the Mobile label. I just wanted to mention that we can avoid adding that label to the list of permitted types 🙂. Thanks! |
…upgrade/react-native-0.71.8
Related PRs
⚙️ Core
🤖 Android
react-native-libraries-publisher:react-nativeto version0.71.8wordpress-mobile/react-native-libraries-publisher#23react-native-reanimatedlibrary with version2.17.0wordpress-mobile/react-native-libraries-publisher#24react-native-gesture-handlerlibrary with version2.10.2wordpress-mobile/react-native-libraries-publisher#25react-native-linear-gradientlibrary with version2.7.3wordpress-mobile/react-native-libraries-publisher#30react-native-linear-gradientwordpress-mobile/react-native-hsv-color-picker#10🍎 iOS
wp-fork-2.17.0branch and main repo's2.17.0version wordpress-mobile/react-native-reanimated#26What?
Upgrades React Native to version
0.71.80.71.11.UPDATE: There were no breaking/major changes between
.8and.11(reference).Why?
This is part of a periodic effort to keep React Native up-to-date in the spirit of having the latest fixes and improvements.
How?
Most of the changes have been applied following the React Native upgrade helper. Here is a list of the different commits applied and some notes:
react-nativedependency to version0.71.8react-nativedependency to version0.71.11@babel/runtimedependency andcocoapodsgemreact-devtools-corepatch to new version@react-navigation/nativepackage to version 6.0.14react-native-reanimatedto version 2.17.0react-native-gesture-handlerto version 2.10.2react-native-linear-gradientto version 2.7.3MockReplaced by Mock return value of Linking addEventListenerLinking.addEventListenerfunctionMediaUploadcomponent testactwarnings produced during block insertionactwarnings in Columns block testsactwarnings in List block testsTesting Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A