-
Notifications
You must be signed in to change notification settings - Fork 273
detectHostComponentNames: render test tree inside act to avoid timer leaks #1371
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
detectHostComponentNames: render test tree inside act to avoid timer leaks #1371
Conversation
f57ca72
to
6096251
Compare
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.
@jsnajdr Looks good! Again thanks for submitting this non-trivial fix!
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1371 +/- ##
=======================================
Coverage 96.13% 96.14%
=======================================
Files 49 50 +1
Lines 3314 3318 +4
Branches 491 492 +1
=======================================
+ Hits 3186 3190 +4
Misses 128 128
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Released in v12.0.1 🎉 |
….1 (#2377) [](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@testing-library/react-native](https://callstack.github.io/react-native-testing-library) ([source](https://togithub.com/callstack/react-native-testing-library)) | [`12.0.0` -> `12.0.1`](https://renovatebot.com/diffs/npm/@testing-library%2freact-native/12.0.0/12.0.1) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>callstack/react-native-testing-library</summary> ### [`v12.0.1`](https://togithub.com/callstack/react-native-testing-library/releases/tag/v12.0.1) [Compare Source](https://togithub.com/callstack/react-native-testing-library/compare/v12.0.0...v12.0.1) #### What's Changed ##### Bugfixes - Flush all pending microtasks and updates before returning from waitFor by [@​jsnajdr](https://togithub.com/jsnajdr) in [https://github.com/callstack/react-native-testing-library/pull/1366](https://togithub.com/callstack/react-native-testing-library/pull/1366) - Render host component name detection tree inside act to avoid timer leaks by [@​jsnajdr](https://togithub.com/jsnajdr) in [https://github.com/callstack/react-native-testing-library/pull/1371](https://togithub.com/callstack/react-native-testing-library/pull/1371) #### New Contributors 👏 - [@​jsnajdr](https://togithub.com/jsnajdr) made their first contributions in [https://github.com/callstack/react-native-testing-library/pull/1366](https://togithub.com/callstack/react-native-testing-library/pull/1366), [https://github.com/callstack/react-native-testing-library/pull/1371](https://togithub.com/callstack/react-native-testing-library/pull/1371) **Full Changelog**: callstack/react-native-testing-library@v12.0.0...v12.0.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/dooboolab/react-native-iap). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xNC4yIiwidXBkYXRlZEluVmVyIjoiMzUuMTQuMiJ9--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
….1 (#2377) [](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@testing-library/react-native](https://callstack.github.io/react-native-testing-library) ([source](https://togithub.com/callstack/react-native-testing-library)) | [`12.0.0` -> `12.0.1`](https://renovatebot.com/diffs/npm/@testing-library%2freact-native/12.0.0/12.0.1) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>callstack/react-native-testing-library</summary> ### [`v12.0.1`](https://togithub.com/callstack/react-native-testing-library/releases/tag/v12.0.1) [Compare Source](https://togithub.com/callstack/react-native-testing-library/compare/v12.0.0...v12.0.1) #### What's Changed ##### Bugfixes - Flush all pending microtasks and updates before returning from waitFor by [@​jsnajdr](https://togithub.com/jsnajdr) in [https://github.com/callstack/react-native-testing-library/pull/1366](https://togithub.com/callstack/react-native-testing-library/pull/1366) - Render host component name detection tree inside act to avoid timer leaks by [@​jsnajdr](https://togithub.com/jsnajdr) in [https://github.com/callstack/react-native-testing-library/pull/1371](https://togithub.com/callstack/react-native-testing-library/pull/1371) #### New Contributors 👏 - [@​jsnajdr](https://togithub.com/jsnajdr) made their first contributions in [https://github.com/callstack/react-native-testing-library/pull/1366](https://togithub.com/callstack/react-native-testing-library/pull/1366), [https://github.com/callstack/react-native-testing-library/pull/1371](https://togithub.com/callstack/react-native-testing-library/pull/1371) **Full Changelog**: callstack/react-native-testing-library@v12.0.0...v12.0.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/dooboolab/react-native-iap). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xNC4yIiwidXBkYXRlZEluVmVyIjoiMzUuMTQuMiJ9--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Fixes a bug that I discovered while working on #1366: when first calling
render
in a test suite, therender
function callsdetectHostComponentNames
, which mounts a testing tree with a few components. This tree is rendered outside theact
environment, and therefore schedules asetImmediate
callback to process effects. This scheduled callback then has an unwanted interaction with my actual testing code. If there is awaitFor
inside my test, the effects scheduled by my testing code will not be processed by mysetImmediate
, but by the one scheduled bydetectHostComponentNames
. That makes the precise order of events inside my test non-deterministic and unpredictable.Look at this call stack:
and note how it starts in my
waitFor.test.ts
test, in therender
function, then callsdetectHostComponentNames
, schedulessetImmediate
callback, and how thatsetImmediate
later processes an update scheduled by my test code (theApple
component).This PR avoids that by wrapping the render with
act
. That way, all effects scheduled by the render will come into theactQueue
, and will be flushed before returning. No leaks.In the code, I had to do some silly IIFE magic to make TypeScript happy: it doesn't know that
act
will synchronously and reliably initialize theresult
value.