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 Support for Device Shake Action #551

Closed
grabbou opened this issue Feb 1, 2018 · 28 comments
Closed

Add Support for Device Shake Action #551

grabbou opened this issue Feb 1, 2018 · 28 comments

Comments

@grabbou
Copy link

grabbou commented Feb 1, 2018

Hey,

I am working on facebook/react-native#17806 where I am attempting to remove Appium and start using Detox for internal tests of React Native as well as the release process.

The problem I am facing right now is lack of the ability to open up Dev Settings (in order to test whether Chrome Debugger or Live Reload is working).

I've done some research and found that one of the ways to do it would be to expose an action on device, called toggleDevSettings that could execute show(https://github.com/facebook/react-native/blob/e8eec24706e792314ee574bbf7f7c0066c4f3a7a/React/DevSupport/RCTDevMenu.m#L292) method which is exposed on RCTDevMenu native module.

Alternatively, we could go towards React Native agnostic solution and implement the ability to trigger shake gesture.

I am open to collaborating on this feature as I need it to move my PR forward.

@grabbou
Copy link
Author

grabbou commented Feb 1, 2018

cc: @rotemmiz as we discussed this on Slack

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 4, 2018

I think a device.shake() API is the correct way to go. iOS simulator has this option in the menu, so it could be possible to use that somehow. If not, it should be possible to implement using some Earl Grey trick or private API directly.

@LeoNatan LeoNatan changed the title Ability to open up Dev Settings Add Support for Device Shake Action Feb 5, 2018
LeoNatan added a commit that referenced this issue Feb 5, 2018
This works by sending a Darwin notification that the simulator sends when simulating shake.
Native implementation needed for #551
@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

Added support for shake in iOS native.
Need JS support from mr. RMM @rotemmiz

@grabbou
Copy link
Author

grabbou commented Feb 5, 2018

Thanks @LeoNatan, any ideas when Android support might land?

@grabbou
Copy link
Author

grabbou commented Feb 5, 2018

How would you attempt using that feature w/o a JS interface? Any workaround I can do to use it right now?

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

I cannot say when Android support might be added.

Regarding a workaround, I am not sure. The shake event that triggers the menu is generated from native. You could drill down in the JS API and somehow call that action directly, but that like implementing the actual JS API (if you do, please submit PR 😄).

@grabbou
Copy link
Author

grabbou commented Feb 5, 2018 via email

@rotemmiz
Copy link
Member

rotemmiz commented Feb 5, 2018

Android, me.

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

@rotemmiz According to this, you can fake it with adb.
That's the hardware menu key.

@rotemmiz
Copy link
Member

rotemmiz commented Feb 5, 2018

adb shell input keyevent 82 is not exactly shake, it's keycode for menu button. It works, but this is not "shake"

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

True, it would be added as a separate action.

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

For shake, I see some need to telnet into the simulator and change the accelerator. 😓

Why it's always harder on Android? 😂

@grabbou
Copy link
Author

grabbou commented Feb 5, 2018 via email

@rotemmiz
Copy link
Member

rotemmiz commented Feb 5, 2018

We already have a pretty good telnet client, not gonna be very hard I hope.

Android has no shake property, it's not a thing in the official API... There is one good library that does that.
They do though support mocking every hardware/software key available (KEYCODE_MENU, etc). Not everything is harder, but if we impose iOS APIs on Android that's what we get.

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

So why not keep shake to iOS and menu for Android?

@rotemmiz
Copy link
Member

rotemmiz commented Feb 5, 2018

Not the same thing, it will not work the same outside of react native

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

But seems to me like those are two separate functionalities unrelated to each other. We could add shake now for iOS and menu only for Android (no menu on iOS), and then add shake later for Android (when necessary).

@rotemmiz
Copy link
Member

rotemmiz commented Feb 5, 2018

I'm starting to think we need another abstraction layer, for react native related operations, that will use different driver APIs under the hood
😕

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

I disagree. Detox is React Native neutral, and should remain that way.

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

(reloadReactNative is also problematic)

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

Further, dev menu is a debug feature, so it makes even less sense in adding it as official API for device. Normally, users run in release.

@rotemmiz
Copy link
Member

rotemmiz commented Feb 5, 2018

That's exactly my point, we don't want to add per platform react native specific behaviors, so we create a dedicated API for react native apps (reloadReactNaive will be added there as well), and native apps will not have these options as part of the API.
reloadReactNative or openDevMenu are only RN options.

Native APIs will include the available native commands for each platform.

I will try to draft something

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

Other than this narrow use-case of testing the tools themselves, why should the dev menu be exposed?
I dislike adding support for the kitchen sink.
On iOS, support for shake is something that is useful beyond dev menu. It's a used gesture for undo, and is often used. So adding support for that has merit. So is the menu button for Android. I just don't see it for RN.

@LeoNatan
Copy link
Contributor

LeoNatan commented Feb 5, 2018

With these two APIs, for this narrow use-case, a workaround can be added to ask if(iOS) shake() else menu() inside @grabbou 's test..

@Salakar
Copy link

Salakar commented Feb 28, 2018

Not sure if this is the best place to chime in but I'd also like to have the ability to be able to control the Dev Settings in RN through detox - at least the ability to toggle remote debugging and to reload react native.


@LeoNatan I have a bit of a weirder use case here though than just tooling tests: whilst detox is great for controlling all things device/UI (which i'm internally still using) I needed a way of testing native modules e2e without extensive tests in native obj-c/java code or without having the tests in-app.

image

As you can see in the above screenshot, this is just standard Node.js + mocha tests but I can now require native modules and use them fully - no mocking, bundle runs in-process automatically (as long as remote debugging is enabled and I have the ability to reload react native - this is where detox comes in).

I plan to bundle this at some point into an NPM package to ease native module testing as there's only a handful of native modules doing testing well/extensively that I could see - I see this as a first step towards simplifying the testing process for native module developers.


@rotemmiz: That's exactly my point, we don't want to add per platform react native specific behaviors, so we create a dedicated API for react native apps (reloadReactNaive will be added there as well), and native apps will not have these options as part of the API.
reloadReactNative or openDevMenu are only RN options.

Agree on this.


@grabbou usage without a JS interface currently implemented - this could be done with a custom action I'd imagine, something I covered here with an example: #207 (comment) (it's a slightly hacky but if it helps...)

@LeoNatan
Copy link
Contributor

@Salakar I think we should move the discussion to #207. I would like to hear more how you have implemented your solution and perhaps discuss how to add the functionality in Detox.

rotemmiz pushed a commit that referenced this issue Mar 10, 2018
* Add support for ShakeDevice action.

This works by sending a Darwin notification that the simulator sends when simulating shake.
Native implementation needed for #551

* Complete comment in code

* Add the test project to Xcode as well

* Add shake action

* Add shake test to suite

* Link to Earl Grey PR in comment

* WAT

* Fix compilation error and remove “it.only” from shake test

* Restore to spaces

* Change shake to support devices as well as simulators.

* reformatting :(

* fixed unit tests

* merged shake tests inside device tests
@LeoNatan
Copy link
Contributor

Device shake has landed for iOS.

@grabbou
Copy link
Author

grabbou commented Mar 20, 2018 via email

@wix wix locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants