Detox icon indicating copy to clipboard operation
Detox copied to clipboard

Disable RN Timers and Animation synchronization to prevent tests from stalling

Open leanmazzu opened this issue 6 years ago • 48 comments

Is your feature request related to a problem? Please describe. It's related to a problem we've been facing at Skyscanner. Some of our tests were stalling in Android because of ReactNativeTimersIdlingResource.isIdleNow() and AnimatedModuleIdlingResource.isIdleNow() never returning true in some cases. I couldn't identify exactly when this happens since some of the times this happened we didn't have any infinite animations running in that particular screen.

Describe the solution you'd like We already have the disableSynchronization() function but that's only disabling the network synchronization. So my proposal is to add similar methods to disable RN timers synchronization and animation synchronization. You can see an already implemented version of this here: https://github.com/leanmazzu/Detox/commit/efb0dcfd799ffea3d91bd1c8e46ee84df3950dd6. This is what we currently have in place in our Skyscanner fork and it's working just fine for us.

Describe alternatives you've considered N/A

Additional context This would be a non-breaking change as it's not modifying the current disableSynchronization() method or anything else.

leanmazzu avatar Jul 08 '19 16:07 leanmazzu

Hmm disableSynchronization() should disable all synchronization. @rotemmiz @d4vidi Why is this not the case in Android?

@leanmazzu Feel free to submit a PR that implements this.

LeoNatan avatar Jul 08 '19 17:07 LeoNatan

Cool will do @LeoNatan , just one question... the solution I proposed above is only adding the functionality to disable ReactNativeTimersIdlingResource and AnimatedModuleIdlingResource. That was enough for us but, do you think it's worth adding the functionality to disable ReactNativeUIModuleIdlingResource as well? Looks like it could help solve this issue https://github.com/wix/Detox/issues/1435

leanmazzu avatar Jul 09 '19 08:07 leanmazzu

I think a disableSynchronization() option should disable everything, like it does on iOS. Waiting for @rotemmiz

LeoNatan avatar Jul 09 '19 08:07 LeoNatan

It should, and if it doesn't we need to fix it. Also, we should probably add per IR disable mechanism (as much as we can)

rotemmiz avatar Jul 09 '19 09:07 rotemmiz

We don't have that mechanism on iOS either for now. But at least the main disable should disable 😄

@leanmazzu We'd be happy to receive a pull request. Thank you!

LeoNatan avatar Jul 09 '19 09:07 LeoNatan

Ok cool, I'll do this then:

  • disableSynchronization() will now disable all the IR.
  • There will be a disable method for every IR so that they can be disabled/enabled separately.

Let me know if I'm missing something here. Otherwise, I'll send the PR implementing this as soon as it's ready. Thank you!

leanmazzu avatar Jul 09 '19 10:07 leanmazzu

Sounds good. Thanks!

LeoNatan avatar Jul 09 '19 10:07 LeoNatan

Sounds about right. Nevertheless @leanmazzu I'm highly intrigued with what your use cases are, I can't help but think whether possibly you're just struggling with an actual bug in one of the idling resources. Either way, 'love the Skyscanner app :)

d4vidi avatar Jul 10 '19 14:07 d4vidi

Usually what we've found on iOS is that people disable sync when they have endless animations or some network. For network, we have a way to disable sync on iOS. Not sure if the same API works for Android (@rotemmiz ?). Regarding animations, we usually recommend mocking the animation calls to only run once or not at all during Detox tests.

LeoNatan avatar Jul 10 '19 15:07 LeoNatan

Network disable/enable sync is implemented in Android, same API. https://github.com/wix/Detox/blob/master/detox/test/e2e/14.network.test.js#L29

rotemmiz avatar Jul 10 '19 15:07 rotemmiz

@rotemmiz I meant this: https://github.com/wix/detox/blob/master/docs/APIRef.DeviceObjectAPI.md#deviceseturlblacklisturls

But I see setURLBlacklist() is used in the tests. 👍

LeoNatan avatar Jul 10 '19 15:07 LeoNatan

Thank you all guys! The PR is ready to review (https://github.com/wix/Detox/pull/1521)

Sounds about right. Nevertheless @leanmazzu I'm highly intrigued with what your use cases are, I can't help but think whether possibly you're just struggling with an actual bug in one of the idling resources. Either way, 'love the Skyscanner app :)

@d4vidi in our case we have some screens with complex view hierarchies. We tried mocking all the animations, RN even provides a way of doing this. But that didn't work for us, as some components were not working at all with animations disabled. Anyways, I'm glad you like Skyscanner app :)

leanmazzu avatar Jul 11 '19 09:07 leanmazzu

That RN feature is completely broken. On iOS, we force it disabled so that apps work. It's not intended for e2e testing.

LeoNatan avatar Jul 11 '19 09:07 LeoNatan

It seems I am hitting the same problem, I get very randomly below errors after test job timeouts: (node:19198) UnhandledPromiseRejectionWarning: Error: Looped for 1978 iterations over 60 SECONDS. The following Idle Conditions failed.

detox[22054] WARN: [Client.js/PENDING_REQUESTS] App has not responded to the network requests below: (id = 110) invoke: {"target":{"type":"Class","value":"com.wix.detox.espresso.EspressoDetox"},"method":"perform","args":[{"type":"Invocation","value":{"target":{"type":"Class","value":"androidx.test.espresso.Espresso"},"method":"onView","args":[{"type":"Invocation","value":{"target":{"type":"Class","value":"com.wix.detox.espresso.DetoxMatcher"},"method":"matcherForTestId","args":["back_button"]}}]}},{"type":"Invocation","value":{"target":{"type":"Class","value":"com.wix.detox.espresso.DetoxViewActions"},"method":"click","args":[]}}]} That might be the reason why the test "Teddy scenario Explore" has timed out.

Do you guys think these logs are related? This seems to happen after tapping buttons that trigger an animation

disableSynchronization does nothing at all for me, not even turning off network. I also tried setURLBlacklist or detoxURLBlacklistRegex without any results

maxime-helen avatar Aug 14 '19 17:08 maxime-helen

I'm hitting a similar issue, UIManagerModule is always busy (because of a looping animation). Was about to implement disable myself but found this! Thanks @leanmazzu for the PR!

@maxime-helen I found it useful to check the instrumentation app logs: adb logcat *:S Detox:V -T 50 If your app is stuck and those actions are timing out then check the logs for messages like "UIManagerModule is busy"

mnzaki avatar Sep 16 '19 19:09 mnzaki

Will this be merged any time soon? Currently blocked on running android tests as well on RN 0.64. My tests start up by typing is about 1 character per 2 seconds, which causes timeouts. I also see a "UIManagerModule is busy" printed quite frequenty b/w characters appearing

chevonc avatar Sep 20 '19 23:09 chevonc

@rotemmiz let's give this high prio, I don't think there's too much left to be done here

d4vidi avatar Sep 22 '19 09:09 d4vidi

heads up: I'm not actively working on this as my team's priorities have shifted a little, especially since the branch in its current state works great for us (thanks @leanmazzu).

mnzaki avatar Oct 01 '19 13:10 mnzaki

Is there any update on this? I'm finding since we upgraded our app to Wix3, this issue started reproducing. I get the Network is busy line when doing logcat, the waiting is a lot worse on Android than on iOS.

j320 avatar Jan 20 '20 16:01 j320

@leanmazzu hey, did you open a PR for this in the end? I am experiencing the same issue

ronilitman avatar Feb 10 '20 12:02 ronilitman

@ronilitman he initially has, but it's become obsolete after requiring various changes. If you wanna take it we'd be happy to accept it.

d4vidi avatar Feb 16 '20 11:02 d4vidi

Thank you all guys! The PR is ready to review (#1521)

Sounds about right. Nevertheless @leanmazzu I'm highly intrigued with what your use cases are, I can't help but think whether possibly you're just struggling with an actual bug in one of the idling resources. Either way, 'love the Skyscanner app :)

@d4vidi in our case we have some screens with complex view hierarchies. We tried mocking all the animations, RN even provides a way of doing this. But that didn't work for us, as some components were not working at all with animations disabled. Anyways, I'm glad you like Skyscanner app :)

These RN files are totally broken, they don't preserve animation behaviors (like call a callback or setting final values when they finish), I've made a patch to apply in my project for them (see the link), at least with it my tests on android were working again

Grohden avatar Mar 18 '20 13:03 Grohden

@Grohden kudos for getting that fixed - even though that in essence, I advise not to disable animations in E2E-level testing. Rather, in our projects, we've force-disabled animations mocking altogether: https://github.com/facebook/react-native/issues/27010#issuecomment-573645965

d4vidi avatar Mar 22 '20 08:03 d4vidi

I have this issue, my use case is as follows (and I'd happily accept guidance to perhaps mitigate it):

  1. Start a single-screen test-only app that just refreshes with status in order to run a big queue of tests, no UI interaction is needed
  2. Apparently (? open question) if there is no UI interaction the tests crawl because it appears that Detox requires UI interaction in order to move quickly?
  3. So we send a steady stream of "adb shell input tap" events to the emulator and things move along, but...
  4. If react-native catalyst instance goes down in between an android MotionEvent.ACTION_DOWN and MotionEvent.ACTION_UP, then it only registers the ACTION_UP, and that apparently leaves something on the queue as detected by UIModuleIdlingResource here, so that queue is permanently blocked (this could be a react-native bug as well then, I suppose)
  5. So now the app is stuck and never sends the testee-side login / ready event, so the test never starts

So in my use case, I don't care at all about synchronization, I'd like to disable it completely on Android, exactly as the API disableSynchronization would indicate.

@d4vidi are you still interested in someone picking up the android disableSynchronization PR? It seems at the least, disable should...disable all synch (not just network) ?

Is there some way to make detox pick up events and keep moving, without UI interaction? Perhaps I'm missing some timing config?

Is anyone familiar with react-native MotionEvent handling and why a broken pair of ACTION_DOWN/ACTION_UP events might jam up the UIManager queue?

Cheers

mikehardy avatar Aug 12 '20 19:08 mikehardy

I am having tests that are failing in Android with UI Kitten modals. I think it is an infinite animation when I open modals Is there any alternative to avoid detox stalling? I tried disableSynchronization() but this only works for iOS...

https://github.com/akveo/react-native-ui-kitten/issues/1250

napsta32 avatar Sep 24 '20 07:09 napsta32

@napsta32 depending on your needs, you can chop any part of Detox out. I completely cut off the UIManagerIdlingResource from being able to block testing for react-native-firebase because for my case I wasn't driving the UI, I was just running effectively headless tests yet the UIIdlingResource has some bug that causes it to halt anyway.

So you just cut it out as a patch-package patch like so https://github.com/invertase/react-native-firebase/blob/master/tests/patches/detox%2B16.7.2.patch

If you're driving the UI then this won't work, but it's an example of bending Detox to your will for your situation

mikehardy avatar Sep 24 '20 13:09 mikehardy

Thanks @mikehardy

This solution is working for my UI tests (somehow) I guess if I have any issues I will try to fix the disableSynchronization() UIManagerIdlingResource bug

napsta32 avatar Sep 28 '20 08:09 napsta32

There was a PR created (https://github.com/wix/Detox/pull/1521) to fix this a long time ago but for some reason was never merged. We have a similar issue where we have an animation loop (custom input blinking cursor) on certain screens that there is just no good way to test on those screens without hacks. Disable animation synchronization on that screen would be a huge help, works on iOS but not on android

martintreurnicht avatar Dec 01 '20 23:12 martintreurnicht

@martintreurnicht , maybe I can look at #1521 next week, but if you feel like you can resurrect and finish the work, you are more than welcome to! 👍

noomorph avatar Dec 02 '20 08:12 noomorph

@d4vidi, please advise if a better solution than #1521 would actually be working on debugSynchronization for Android? I'm not 100% sure what our stance on it is.

/cc @rotemmiz

noomorph avatar Dec 02 '20 08:12 noomorph