[RNMobile] Silence act warnings triggered after test finishes
#38336
Conversation
fluiddot
added
[Type] Bug
|
Size Change: 0 B Total Size: 1.14 MB ℹ️ View Unchanged
|
fluiddot
force-pushed
the
rnmobile/fix/act-warnings-after-tests
branch
from
8f5ce79 to
f9bb733
Compare
There was a problem hiding this comment.
Thank you for continuing to debug this. This particular issue is a complex one.
@dcalhoun I also checked that this approach works by disabling the use of the fake timers workaround we introduced in this PR, however, I wanted first to check with you whether you think we could proceed to remove it, thanks 🙇 .
Given the act warnings reappeared in #38332, it is obvious that the fake timers did not solve the problem entirely. However, I still wonder if they solved at least some of the act warning origins.
To my understanding, the changes in this PR effectively silence all end-of-test act warnings, so it makes sense that it "works" without the fake timers. The PR "working" does not, however, directly negate the fake timers (seemingly) successfully accounting for timer-based act warnings. (Or maybe the fake timers' prior success was a "red herring" and it was coincidence that the fake timers seemingly resolved the timer-based act warnings. 🙃)
Because of that, I am unsure as to whether it makes sense to remove the fake timers. I am going to debug this more as well. Let me know if you'd prefer to move forward with the stop-gap solution of silencing all end-of-tests act warnings, though.
| // Automatically cleanup mounted React trees after rendering components with the React | ||
| // Native Testing Library. Although this is already done by the library, we provide our | ||
| // own logic in order to prevent unexpected "act" warnings after the test finishes. | ||
| // Reference: https://callstack.github.io/react-native-testing-library/docs/api#cleanup | ||
| afterEach( cleanup ); |
There was a problem hiding this comment.
I interpret this custom implementation of clean up to effectively intentionally ignore valid act warnings, as the clean up now no longer includes await flushMicroTasks(). I.e with this change, we make ourselves vulnerable to introducing additional act warnings. Additionally, the current act warnings may produce false positive tests in the "right" context (not sure what that hypothetical context might be at this moment given we do not fully understand the source of the act warnings). Is that correct?
I understand why we may want to land this temporary solution, but I wanted to clarify the intent. I also wonder if we should reframe the PR title and description to explicitly state we are disabling end-of-test act warnings rather than "fixing" them. The explicitness may prove useful to future readers. WDYT?
There was a problem hiding this comment.
I interpret this custom implementation of clean up to effectively intentionally ignore valid
actwarnings, as the clean up now no longer includesawait flushMicroTasks(). I.e with this change, we make ourselves vulnerable to introducing additionalactwarnings. Additionally, the currentactwarnings may produce false positive tests in the "right" context (not sure what that hypothetical context might be at this moment given we do not fully understand the source of theactwarnings). Is that correct?
Yes, that's correct. Actually, this solution diverges from the original idea of waiting for all resolvers during the editor initialization. As I commented in this comment, it's not totally clear to me whether omitting the React tree updates made by those selectors could really affect the result of the tests. In any case, I think a deeper investigation might help us on both having a better context of what happens during initialization and getting clues on how to provide a better workaround 👍 .
I understand why we may want to land this temporary solution, but I wanted to clarify the intent. I also wonder if we should reframe the PR title and description to explicitly state we are disabling end-of-test
actwarnings rather than "fixing" them. The explicitness may prove useful to future readers. WDYT?
Sure, I'll rename it to make it clearer.
Yeah most likely, I think now the problem is related to the Promise resolving done within the gutenberg/packages/data/src/redux-store/index.js Lines 414 to 427 in 1000aab
This approach doesn't wait for the resolvers to be finished before starting the test. So it's true that it silences them due to the fact that the editor is unmounted before potential selectors get resolved.
From my POV, if we want to assure that all selectors are resolved upon the editor initialization, and before tests, it makes sense to fake timers but I have the gut feeling that we need to wait extra time for the resolvers to be really finished.
I think it makes sense to keep investigating a better approach, as it's still unclear to me what's the impact of the side effects produced by the React tree updates, related to the resolvers that trigger the |
fluiddot
changed the title
act warnings triggered after test finishesact warnings triggered after test finishes
Thank you very much @dcalhoun for investigating deeper this issue, I really appreciate your effort to address this complex problem 🙇 . I've just reviewed #38344 and posted some comments, overall, I think it's a good approach 👍 . |
|
Closing this PR in favor of #38344. |
Description
Fixes #38332.
After investigating further following the findings posted in #38052, I realized that the
actwarnings are caused by this line related to the auto cleanup logic of tests. If the test finishes before any of the Redux selector resolver has finished, React state updates would happen after the test is finished and before the editor is unmounted hence, the React Native Testing library will detect them and display theactwarnings.As a workaround, I overrode in this PR the auto cleanup logic of the React Native Testing library and provided our own implementation, based on the examples provided by the library in relation to the
cleanupfunction.UPDATE: This approach is based on the fact that it's safe to omit the React tree updates, produced by the Redux selectors resolvers triggered during the editor initialization (i.e. the ones that produce the
actwarnings after the test finishes). This means that when initializing the editor we won't wait for those selectors to be finished.Testing Instructions
actwarnings are not displayed following the reproduction instructions from Integration tests:actwarnings can still be triggered when testing the editor in multiple tests #38332.npm run native testand observe that all tests pass.Screenshots
N/A
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).