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.

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

Add an API for Detox to check if there are any timers expiring in a certain range #27539

Closed
wants to merge 1 commit into from

Conversation

@ejanzer
Copy link
Contributor

ejanzer commented Dec 16, 2019

Summary

Detox currently relies on reflection to inspect the private timers queue in the Timing module to check if React Native is idle or not. This broke when I renamed TimingModule and moved the queue to JavaTimerManager in D17260848 and D17282187. A better solution to this problem is for us to expose a public API for checking the timers queue from TimingModule, so that Detox doesn't need to access private fields.

Using similar logic to Detox's TimersIdlingResource: https://github.com/wix/Detox/blob/4f81a77baee4e4542a693922bcbde621d9d8c1a5/detox/android/detox/src/main/java/com/wix/detox/reactnative/idlingresources/TimersIdlingResource.kt#L95
based on discussion here: wix/Detox#907

Changelog

[Android] [Added] Added an API for checking if there are busy timers to TimingModule

Test Plan

Added tests to TimingModuleTest.java

Differential Revision: D19128786

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Dec 16, 2019

This pull request was exported from Phabricator. Differential Revision: D19128786

ejanzer added a commit to ejanzer/react-native that referenced this pull request Dec 16, 2019
…ertain range (facebook#27539)

Summary:
Pull Request resolved: facebook#27539

Detox currently relies on reflection to inspect the private timers queue in the Timing module to check if React Native is idle or not. This broke when I renamed TimingModule and moved the queue to JavaTimerManager in D17260848 and D17282187. A better solution to this problem is for us to expose a public API for checking the timers queue from TimingModule, so that Detox doesn't need to access private fields.

Using similar logic to Detox's TimersIdlingResource: https://github.com/wix/Detox/blob/4f81a77baee4e4542a693922bcbde621d9d8c1a5/detox/android/detox/src/main/java/com/wix/detox/reactnative/idlingresources/TimersIdlingResource.kt#L95

Changelog: [Android] [Added] Added an API for checking if there are busy timers to TimingModule

Differential Revision: D19128786

fbshipit-source-id: 5e5e09043931836dcab56b174d7f26df935519d6
@ejanzer ejanzer force-pushed the ejanzer:export-D19128786 branch from acac54d to b7ca64c Dec 16, 2019
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Dec 16, 2019

This pull request was exported from Phabricator. Differential Revision: D19128786

ejanzer added a commit to ejanzer/react-native that referenced this pull request Dec 16, 2019
…ertain range (facebook#27539)

Summary:
Pull Request resolved: facebook#27539

Detox currently relies on reflection to inspect the private timers queue in the Timing module to check if React Native is idle or not. This broke when I renamed TimingModule and moved the queue to JavaTimerManager in D17260848 and D17282187. A better solution to this problem is for us to expose a public API for checking the timers queue from TimingModule, so that Detox doesn't need to access private fields.

Using similar logic to Detox's TimersIdlingResource: https://github.com/wix/Detox/blob/4f81a77baee4e4542a693922bcbde621d9d8c1a5/detox/android/detox/src/main/java/com/wix/detox/reactnative/idlingresources/TimersIdlingResource.kt#L95

Changelog: [Android] [Added] Added an API for checking if there are busy timers to TimingModule

Differential Revision: D19128786

fbshipit-source-id: 2e7fc7310b1ea5ce90a8e65d5f57d62d994e0eb6
@ejanzer ejanzer force-pushed the ejanzer:export-D19128786 branch from b7ca64c to 449b0d6 Dec 16, 2019
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Dec 16, 2019

This pull request was exported from Phabricator. Differential Revision: D19128786

…ertain range (#27539)

Summary:
Pull Request resolved: #27539

Detox currently relies on reflection to inspect the private timers queue in the Timing module to check if React Native is idle or not. This broke when I renamed TimingModule and moved the queue to JavaTimerManager in D17260848 and D17282187. A better solution to this problem is for us to expose a public API for checking the timers queue from TimingModule, so that Detox doesn't need to access private fields.

Using similar logic to Detox's TimersIdlingResource: https://github.com/wix/Detox/blob/4f81a77baee4e4542a693922bcbde621d9d8c1a5/detox/android/detox/src/main/java/com/wix/detox/reactnative/idlingresources/TimersIdlingResource.kt#L95

Changelog: [Android] [Added] Added an API for checking if there are busy timers to TimingModule

Reviewed By: makovkastar

Differential Revision: D19128786

fbshipit-source-id: 5584e5799eaff77c6ac90e45d2a7843347c683a0
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Dec 18, 2019

This pull request was exported from Phabricator. Differential Revision: D19128786

@ejanzer ejanzer force-pushed the ejanzer:export-D19128786 branch from 449b0d6 to f6903bd Dec 18, 2019
@ejanzer

This comment has been minimized.

Copy link
Contributor Author

ejanzer commented Jan 6, 2020

Hey @d4vidi, mind taking a look at this PR? I want to make sure this is something that we'd actually want to use for Detox before I land it. Some additional discussion/context with @rotemmiz here: wix/Detox#1801 (comment)

@LeoNatan

This comment has been minimized.

Copy link
Contributor

LeoNatan commented Jan 6, 2020

Hello,
Another thing to consider, if possible, is to create an event-based API, rather than a polling API.

On iOS, we are rewriting Detox to be fully event-based, and have thus dropped Earl Grey. On iOS, whenever a new timer (that is to be accepted) is started, we increment a counter and once timer expires, we decrement the counter. Once counter is zero, the sync resource is considered free. Otherwise busy.

On Android, Espresso already has support for event based idle resources, and some resources already are such. In the future, we may consider converting our polling idle resources to event based, and it would be great to have such an API available for use. Perhaps this can be the first resource to convert. An observer patter over timers state would work great here.

Thanks 🙂

@rotemmiz

This comment has been minimized.

Copy link
Contributor

rotemmiz commented Jan 6, 2020

In addition to what @LeoNatan said, event based IdlingResources will greatly decrease the CPU utilization of Detox, and will enable more test concurrency using the same hardware. My assumption is that the new implementation will open the possibility to use twice the worker count when running Detox (currently we use 3 workers on 8-vcore VMs, I believe the new implementation will give us the ability to run 5-6 workers with the same hardware).

@ejanzer

This comment has been minimized.

Copy link
Contributor Author

ejanzer commented Jan 6, 2020

@LeoNatan / @rotemmiz : Correct me if I'm wrong, but it sounds like the reason why Detox needs the number of active timers is just to tell whether JS is idle, right? If we're just looking for a proxy for whether JS is idle, or whether the UI is done updating, then I think we could expose that information more directly - we could create an idling resource that would notify you when the JS runtime itself is free, for example. This is something that I'm going to need as I'm trying to write tests for RN running in bridgeless mode, so I'd probably want to do this anyway. If that would meet Detox's needs, then I probably wouldn't want to rewrite timers to use an idling resource right now, since I'd just be replacing it soon anyway.

If you do actually need the timers themselves for some reason, then I think it makes sense to use an event-based resource like you described. I don't really have time to do it right now, but I'm happy to look at any PRs that make the change, if anyone else wants to jump in! (The motivation for this PR was just to unbreak Detox on the current RN RC, because I broke it by refactoring the timers module).

@LeoNatan

This comment has been minimized.

Copy link
Contributor

LeoNatan commented Jan 6, 2020

We’ve found creative uses for the information. While the Detox iOS rewrite is ongoing, we’re now using the same sync management system in Detox Instruments to provide app activity information to the user:

https://github.com/wix/DetoxSync
https://github.com/wix/DetoxInstruments/blob/master/Documentation/Instrument_Activity.md

It’s an unintended consequence, but very useful indeed. The sync manager I’ve developed has lit up a lot of useful information, all from the same sync points as Detox uses, including the timers.

@d4vidi

This comment has been minimized.

Copy link
Contributor

d4vidi commented Jan 7, 2020

@ejanzer first of all, thanks for this!
Second, in Detox, indeed we do have interest in telling whether there are timers waiting in queue, so as to consider the system 'busy' even though the JS itself is idle (namely, whenever there are non-repeating timers within a 1.5sec look-ahead window - hence the logic in our implementation).
The reasoning behind this is to avoid moving to the next test-step (e.g. tapping on a button), as it can trigger a time-consuming operation that would be interrupted by the expiring pending timeout.

So to answer your question, once we decide to switch to the event-based world, we would have to have an event-based idling-resource interrogator implementation specific to timers. As @LeoNatan explained, it shouldn't be hard to achieve as we have the concept already working on iOS, and also Espresso is ready for this as a framework.

@ejanzer

This comment has been minimized.

Copy link
Contributor Author

ejanzer commented Jan 7, 2020

@d4vidi Ah, I see, yeah that makes sense. So what would you like to do with this PR? Should I abandon it since we want to move to an event-based world soon anyway? Or land it as a temporary measure?

@ejanzer

This comment has been minimized.

Copy link
Contributor Author

ejanzer commented Jan 7, 2020

Nevermind, just caught up with the discussion over in wix/Detox#1801 - sounds like we're good to go with this PR for now. I'll land it.

@react-native-bot

This comment has been minimized.

Copy link
Collaborator

react-native-bot commented Jan 7, 2020

This pull request was successfully merged by @ejanzer in 22764e6.

When will my fix make it into a release? | Upcoming Releases

*/
/* package */ boolean hasActiveTimersInRange(long rangeMs) {
synchronized (mTimerGuard) {
for (Timer timer : mTimers) {

This comment has been minimized.

Copy link
@d4vidi

d4vidi Jan 13, 2020

Contributor

Hey @ejanzer! I hope I'm not too late - seeing this has already been closed/merged, I'd like to request some changes nevertheless.

Here, in order to evaluate whether active timers exist, the timers collection gets iterated over. If you take a closer look into detox' implementation, we do eventually run the same logic, but in addition we first attempt to inspect the first (i.e. least) timer in the collection. That is done for a reason:

As the collection is in fact a PriorityQueue, these facts hold true:

  1. There's no indexed access to items in it - only access to the least item using peek() and poll().
  2. (To quote from PriorityQueue.iterator()) The iterator does not return the elements in any particular order.

These 2 combined evidently mean that most of the time, the least item is in fact the one we're looking for, as its interval is the lowest, and also that iteration is inefficient. We therefore resolve to this order-less iteration only "if we have to", namely, if the least item happens to be a repeating one.

Actually, I see now that an additional optimization can be applied: if the least item is a nonrepeating one that has a long interval, the iteration can be avoided as well, as all other timers' intervals are longer by-design.

So, given this tedious explanation..... do you think you could apply these existing optimizations to your implementation? 😄

This comment has been minimized.

Copy link
@ejanzer

ejanzer Jan 16, 2020

Author Contributor

Ah I see, I should have looked at that a little more carefully! So to reiterate, we're going to skip iterating over the entire timers queue if the least timer (timers.peek()) is nonrepeating and its interval is greater than the specified threshold.

That's the only change to make, right? Or were there any other optimizations that I missed?

This comment has been minimized.

Copy link
@d4vidi

d4vidi Jan 20, 2020

Contributor

Yes! except for:

interval is greater than the specified threshold

interval should be lower (i.e. within the time-window's bounds)

In any case, I'm gonna go ahead and merge + release the Detox changes so you could start working with an actual Detox that does what you need!

This comment has been minimized.

Copy link
@ejanzer

ejanzer Jan 22, 2020

Author Contributor

Thanks @d4vidi! I'll send a PR today with this change.

This comment has been minimized.

Copy link
@ejanzer

ejanzer Jan 22, 2020

Author Contributor

Adding in #27841

ejanzer added a commit to ejanzer/react-native that referenced this pull request Jan 22, 2020
…efore iterating

Summary:
Follow up from facebook#27539 - adding back in the optimization that Detox has in TimersIdlingResource to avoid iterating over the entire timers queue if the next timer is already within the specified range.

Changelog: [Internal]

Differential Revision: D19522346

fbshipit-source-id: d2b4441f56716981d0d6d59ef3ade221eeef5adb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.