Detox: Incompatibility with Android RN master TimingModule

[rod-salazar]

Describe the bug
FYI: It looks like at least 2 things the Detox android implementation depends on are no longer true in RN master in regards to the timers implementation.

• That there's a class called com.facebook.react.modules.core.Timing. It's been renamed to TimingModule (changed in https://github.com/facebook/react-native/commit/e9b50fa4ee087ea0f2f09c01a0a9c1d85a638f54#diff-591535399526875d82f1fcd15406678a)
• That the timing module class impl has a field named 'mTimers'. The module has been refactored to hold this timer information in an instance of a new class called JavaTimerManager. (changed in https://github.com/facebook/react-native/commit/4d774bdc0db0547fec5123ebb88e1994b271271c#diff-591535399526875d82f1fcd15406678a)

To Reproduce
Test Detox against react native master and run into these build issues.

cc @cs01

[d4vidi]

@rodrigos-facebook wow, a big thank-you for this!!! We'll make the proper adaptation.
As I understand it (looking at the commits log) we ought to expect these changes to apply starting RN .62.0, right?

[cs01]

FYI we have added internal land-blocking tests at facebook that ensure iOS compatibility between Detox and React Native. We are planning to add similar tests for Android once a new Detox version with fixes for the issues Rodrigo described have been published. By doing this we hope to avoid landing React Native changes to master that break the React Native/Detox interface, or at least be more proactive in fixing them before landing breaking changes in the React Native codebase.

[d4vidi]

This is very exciting. Great to hear!

[ejanzer]

I've put up a PR to add a new API to TimingModule that should expose the information that Detox needs: https://github.com/facebook/react-native/pull/27539

If that API does what we need, then we should be able to use that instead of updating Detox to refer to the new class name. I basically just copied what Detox does in TimersIdlingResource based on the discussion in #907.

[rotemmiz]

Just a thought about everything going on in here (at 01:04 AM), if we are already getting help from Facebook on this, setting up RN to fit Detox's needs, we might as well want to think of official espresso Idling resources like APIs, which are event based and not polling based, integrating idling resources inside RN itself, like described here https://developer.android.com/training/testing/espresso/idling-resource#integrate-recommended-approach

Disclaimer: I haven't read the whole thread or web over the PR, but want to make sure we're moving in the recommended espresso way. I will read everything and revise this comment if needed.

[ejanzer]

@rotemmiz

I'd definitely be interested in looking into using proper idling resources in RN for in the future, but I don't think I want to try to make that change right now - I'm working on rewriting some pieces of our infra to no longer use the bridge, so I'd probably try to make that change as part of that effort, rather than refactoring our existing code. With this PR I was just trying to do a simple fix so that Detox doesn't have to rely on reflection (since I broke that by renaming + refactoring the Timing class).

What do you think? Is that PR a good step forward for now, or would you rather just keep using reflection for now, and update the class name, etc.?

[rotemmiz]

I'd choose to avoid reflection as much possible, but it's for @d4vidi to decide on this question.

[d4vidi]

@ejanzer There's no real need for reflection here but for the sake of accessing internals. Seems like your changes in https://github.com/facebook/react-native/pull/27539, then, will eliminate usage of reflection altogether!

Anyways again as I already mentioned in https://github.com/facebook/react-native/pull/27539, it's IMO ok to first fix this part, then when we're ready for the transition to event-based implementations... well, we already know what we're up against.

The question I'm asking myself now is whether this can be closed until your https://github.com/facebook/react-native/pull/27539 is merged in, or should we already have this fixed for 0.62 RC's out there?

[TheSavior]

We have product teams at FB using Detox to test against master. Unfortunately this is currently broken because of the timer refactor. While an event based API might be architecturally best, it will take longer to make happen as @ejanzer doesn’t have the bandwidth to work on it.

I’d like for us to move forward if possible with the existing PR to get detox working as quickly as possible against 0.62 so we can reenable our product team’s tests. Any support we could get to do so would be greatly appreciated.

[d4vidi]

@TheSavior I'd be more than happy to move forward -- I wasn't suggesting to wait for the rearchitecture, just basically asking whether we could wait for https://github.com/facebook/react-native/pull/27539 to be merged in so as to avoid reimplementing this twice.

[d4vidi]

Well as https://github.com/facebook/react-native/pull/27539 has been merged, it seems, I'll switch to using the newly timers interrogation API by @ejanzer. Cool with everyone?

[TheSavior]

Yep, that's what I meant. Moving forward with the new API seems like a good idea

[d4vidi]

consider it done

[d4vidi]

Released under 15.1.3 @ejanzer