Skip to content
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.

Sign up for GitHub

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

[RNMobile] Fix act warnings that might be triggered after test finishes #38052

Merged
merged 7 commits into from
Jan 19, 2022
Merged

[RNMobile] Fix act warnings that might be triggered after test finishes #38052

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a8b4abb
Fix act warnings after test finishes
fluiddot Jan 18, 2022
6678a20
Update comment of setImmediate workaround
fluiddot Jan 19, 2022
af1d8ca
Unmount editor when test finish
fluiddot Jan 19, 2022
e6ef33d
Update initialize editor helper
fluiddot Jan 19, 2022
a7d9acc
Update tests that use initialize editor
fluiddot Jan 19, 2022
406424f
Merge branch 'rnmobile/fix/act-warnings-rich-text-tests' of github.co…
fluiddot Jan 19, 2022
02848bd
Fix act warnings from store resolvers with fake timers (#38077)
dcalhoun Jan 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 19 additions & 15 deletions test/native/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,35 +35,39 @@ provideToNativeHtml.mockImplementation( ( html ) => {
} );

export function initializeEditor( props ) {
const renderResult = render(
const screen = render(
<Editor
postId={ `post-id-${ uuid() }` }
postType="post"
initialTitle="test"
{ ...props }
/>
);
const { getByTestId } = renderResult;
const { getByTestId } = screen;

// A promise is used here, instead of making the function async, to prevent
// the React Native testing library from warning of potential undesired React state updates
// that can be covered in the integration tests.
// Reference: https://git.io/JPHn6
return new Promise( ( resolve ) => {
waitFor( () => getByTestId( 'block-list-wrapper' ) ).then(
( blockListWrapper ) => {
// onLayout event has to be explicitly dispatched in BlockList component,
// otherwise the inner blocks are not rendered.
fireEvent( blockListWrapper, 'layout', {
nativeEvent: {
layout: {
width: 100,
},
// Some of the store updates that happen upon editor initialization are executed at the end of the current
// Javascript block execution and after the test is finished. In order to prevent "act" warnings due to
// this behavior, we wait for the execution block to be finished before acting on the test.
fluiddot marked this conversation as resolved.
Show resolved Hide resolved
act(
() => new Promise( ( actResolve ) => setImmediate( actResolve ) )
).then( () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without full context of the origin issue, this solution comes across as somewhat "mask" for the core issue rather than a fix. That is not to say it is not a valid solution, I just hope to understand the issue itself as well.

Were you able to identify what specific update occurs? That context would be helpful to understanding the proposed fix. 🙇🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally understand your point, I'm actually gathering insights for providing a full context, I'll let you know when it's ready 🙇 .

Copy link
Contributor Author

@fluiddot fluiddot Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcalhoun I added a Context section in the PR's description regarding this topic, it's a bit hard to explain as it's related to asynchronicity and store updates, so let me know if you'd like me to expand anything.

Probably, the best way to have a better context is by modifying the code and adding some breakpoints, I referenced where I placed mine for testing (reference) in case it helps out, but feel free to test further in this regard.

Copy link
Member

@dcalhoun dcalhoun Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcalhoun I added a Context section in the PR's description regarding this topic, it's a bit hard to explain as it's related to asynchronicity and store updates, so let me know if you'd like me to expand anything.

Fantastic! Thank you so much for taking the time to document this context. It is immensely helpful for me, and I imagine it will be for future readers as well.

Probably, the best way to have a better context is by modifying the code and adding some breakpoints, I referenced where I placed mine for testing (reference) in case it helps out, but feel free to test further in this regard.

Agreed. I will likely circle back to further explore this subject at some point. Specifically, thanks to the wonderful context you provided, I wonder if we might be able to wrap either the resolver promise and/or a manual tick of fake timers with act to explicitly await the core asynchronous work. I.e. I question if Promise + setImmediate is the most straightforward way of expressing what we are awaiting.

I do not share that thought to question the current implementation or imply that we should explore it now, but merely "thinking out loud."

// onLayout event has to be explicitly dispatched in BlockList component,
// otherwise the inner blocks are not rendered.
fireEvent( getByTestId( 'block-list-wrapper' ), 'layout', {
nativeEvent: {
layout: {
width: 100,
},
} );
resolve( renderResult );
}
);
},
} );

resolve( screen );
} );
} );
}

Expand Down