-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Disable RN Timers and Animation synchronization to prevent tests from stalling #1513
Comments
Hmm @leanmazzu Feel free to submit a PR that implements this. |
Cool will do @LeoNatan , just one question... the solution I proposed above is only adding the functionality to disable |
I think a |
It should, and if it doesn't we need to fix it. |
We don't have that mechanism on iOS either for now. But at least the main @leanmazzu We'd be happy to receive a pull request. Thank you! |
Ok cool, I'll do this then:
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! |
Sounds good. Thanks! |
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 :) |
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. |
Network disable/enable sync is implemented in Android, same API. |
@rotemmiz I meant this: https://github.com/wix/detox/blob/master/docs/APIRef.DeviceObjectAPI.md#deviceseturlblacklisturls But I see |
Thank you all guys! The PR is ready to review (#1521)
@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 :) |
That RN feature is completely broken. On iOS, we force it disabled so that apps work. It's not intended for e2e testing. |
It seems I am hitting the same problem, I get very randomly below errors after test job timeouts:
Do you guys think these logs are related? This seems to happen after tapping buttons that trigger an animation
|
I'm hitting a similar issue, @maxime-helen I found it useful to check the instrumentation app logs: |
Will this be merged any time soon? Currently blocked on running android tests as well on |
@rotemmiz let's give this high prio, I don't think there's too much left to be done here |
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). |
Is there any update on this? I'm finding since we upgraded our app to Wix3, this issue started reproducing. I get the |
@leanmazzu hey, did you open a PR for this in the end? I am experiencing the same issue |
@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. |
@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: |
I have this issue, my use case is as follows (and I'd happily accept guidance to perhaps mitigate it):
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 @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 |
I am having tests that are failing in Android with UI Kitten modals. I think it is an infinite animation when I open modals |
@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 |
Thanks @mikehardy This solution is working for my UI tests (somehow) |
There was a PR created (#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 , 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! 👍 |
Well effectively debugSynchronization != disableSynchronization. We have clear plans regarding the former, but none for the latter. I generally think #1521 is a good approach but we don't have a timeline for that yet. |
Sure, i can resurrect it. Doesn't seem like there are any outstanding items there though, seems like it was just abandoned for whatever reason. I'll see if there is any major changes that need to be made once applied to head of master, if it all seems to work ok i'll just open a new PR |
In order for this fix to work, must the test be desynchronized from the start of the test? Ideally, I would love (especially since this is not a problem with my iOS tests) to be able to have the app in sync for some setup steps particular to the test and then have it desynchronize and complete the test. |
@iMaupin that sounds like a (valid) extension of the feature requested here. |
are there any updates? thank you. |
None so far. |
@pradeipp We have a reference document for that. I hope you'd find that helpful. |
Thank you @d4vidi I've already went over that article quite a few times actually. I've been facing issues related to endlessly running Animations and wanted to see if the individual synchronization methods mentioned above were implemented or not. Saw this line in the docs so I'm guessing there's no way to turn off just animation synchronization through a simple api call as of right now?
|
Yes, not at the moment; That statement generally still holds true, although, as noted before -- we could definitely do with help with this from the community. |
This is still happening with sync turned off. Makes detox unusable to us. Is there any way to figure out what the offending animation is? Otherwise the logs give no insight into what is actually hanging. |
@owens-ben you can either cut that part out (my updated patch is here https://github.com/invertase/react-native-firebase/tree/main/tests/patches) or use a similar patch to print out exactly what thing is being waited on in order to expect more closely |
@mikehardy thanks for responding. I implemented your patch but it doesn't seem to help. Can you explain exactly what it's supposed to accomplish? I see errors in my logcat about UIManagerModule and Catalyst not being available but I'm not sure what the cause is, and I see your patch addresses things related to that. I'm trying to use --debug-synchronization 500 to figure out what is hanging in my app. I was thinking maybe it was a request to segment.io so I tried using the blacklist, but that didn't seem to be the issue. I'm still getting errors like this
Do you have any other advice? |
It is important to note that on android you need to integrate detox as a "from source" build (not the normal type of detox integration) in order for the patch to matter My patch just disables one type of synchronization, it may be that the sync you are infinitely waiting on is a different type, I can't say. I don't have any other advice other than to make sure you've de-integrated the pre-built detox and integrated the source build, then to roll sleeves up and see exactly what is being waited on. The debug stuff looks interesting in that there appears to be lots of stuff going on, native code, JS code and a UI render 🤷 |
@mikehardy I mean it's gotta be an "animation" thats reanimating endlessly. Detox has a bunch issues related to disableSynchronization() not working on android and/or for animations. (ex #2799) We do use bottomsheet and one other commenter said that might be it. |
Could this problem have cropped up again? I'm noticing after upgrading to Detox 20.20.0, RN 0.73.6 and Reanimated 3.8.1 that update: Downgrading to an Android 13 (API 33) emulator seems to have helped here. So perhaps there's an idling issue with Android 14? |
@mrbrentkelly This issue is quite old. Can you please report that on a new GH issue? |
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()
andAnimatedModuleIdlingResource.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: leanmazzu@efb0dcf. 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.The text was updated successfully, but these errors were encountered: