-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: Fix failures due to image component modifications #58213
Merged
derekblank
merged 1 commit into
rnmobile/prevent-image-flicker-when-uploading
from
test/fix-image-tests
Jan 24, 2024
Merged
test: Fix failures due to image component modifications #58213
derekblank
merged 1 commit into
rnmobile/prevent-image-flicker-when-uploading
from
test/fix-image-tests
Jan 24, 2024
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
The mobile image component introduce new asynchronous effects. We must await the result of those effect, in the form of asserting UI changes from the subsequent state updates, to satisfy the React test utilities expectation that unexpected re-renders do not occur after the test completes. Additionally, there were instances of awaiting `act` invocations that were not asynchronous. The erroneous usage of `await` for these synchronous `act` calls resulted in cryptic errors. In reality, the logic run within these `act` calls are synchronous and should merely be wrapped in `act` to signal that they result in a re-render.
dcalhoun
added
[Type] Automated Testing
Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Mobile App - i.e. Android or iOS
Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
labels
Jan 24, 2024
Size Change: 0 B Total Size: 1.69 MB ℹ️ View Unchanged
|
derekblank
approved these changes
Jan 24, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you very much for addressing this. 🚀
I see you've re-ran the failed PHP tests a few times, but they appear to be unrelated to these changes. Going to merge into the target branch and we can address them from there.
derekblank
merged commit Jan 24, 2024
553c243
into
rnmobile/prevent-image-flicker-when-uploading
42 of 61 checks passed
derekblank
added a commit
that referenced
this pull request
Jan 31, 2024
…URI (#57869) * Update native image component to avoid flicker when updating the URI * Update non-visible Image block styles * Update mobile Image block to use separate network and local image URL behavior between platforms * Update Image block platform fetching logic * Update Android logic for Image block platform fetching logic * Add further updates to Image block platform logic * Fix code formatting issue in Image block source prop * Resolve RNImage.prefetch promise * Update CHANGELOG * Update non-visible image styles Resolves layout display issue when block is selected * Remove duplicate references to localURL * Fix Replace Image behavior to update from localURL * Refactor Image block URL handling logic * test: Fix failures due to image component modifications (#58213) The mobile image component introduce new asynchronous effects. We must await the result of those effect, in the form of asserting UI changes from the subsequent state updates, to satisfy the React test utilities expectation that unexpected re-renders do not occur after the test completes. Additionally, there were instances of awaiting `act` invocations that were not asynchronous. The erroneous usage of `await` for these synchronous `act` calls resulted in cryptic errors. In reality, the logic run within these `act` calls are synchronous and should merely be wrapped in `act` to signal that they result in a re-render. * Update nonVisibleImage styles Using width: 1 and height: 1 ensures that onLoad will fire * Update packages/components/src/mobile/image/index.native.js Co-authored-by: David Calhoun <[email protected]> * Update RNImage error handler event with code comment * Update selected images styles on iOS to account for non-visible image On iOS, add 1 to height to account for the 1px non-visible image that is used to determine when the network image has loaded We also must verify that it is not NaN, as it can be NaN when the image is loading. --------- Co-authored-by: David Calhoun <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Mobile App - i.e. Android or iOS
Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
[Type] Automated Testing
Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
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?
Fix test failures due to image component changes.
Why?
Passing automated tests ensure we avoid bugs.
How?
The mobile image component introduce new asynchronous effects. We must
await the result of those effects, in the form of asserting UI changes
from the subsequent state updates, to satisfy the React test utilities
expectation that unexpected re-renders do not occur after the test
completes.
Additionally, there were instances of awaiting
act
invocations thatwere not asynchronous. The erroneous usage of
await
for thesesynchronous
act
calls resulted in cryptic errors. In reality, thelogic run within these
act
calls are synchronous and should merely bewrapped in
act
to signal that they result in a re-render.Testing Instructions
N/A, CI checks should pass.
Testing Instructions for Keyboard
N/A, CI checks should pass.
Screenshots or screencast
N/A, no visual changes.