Closed Bug 1456190 Opened Last year Closed Last year

Implement a minimal remote debugging Java client for tests

Categories

(GeckoView :: General, enhancement, P2)

All
Android
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(3 files, 1 obsolete file)

Implement a minimal Java client for the Gecko remote debugging protocol, so that tests can use it to interact with the page under test. Currently, the plan is to implement enough of the protocol to use the `evaluateJS` webconsole command.
The part 1 patch adds a remote debugging protocol client to the GeckoView testing framework. It basically opens a socket to connect to devtools, and implements minimal support to get the actor for the active tab, and the webconsole actor under that. Finally, it implements the `evaluateJS` call and code for interacting with grips.

I think compatibility with devtools will be quite good given the minimal nature of this client. NI'ing Jet to keep the devtools team in the loop.
Flags: needinfo?(bugs)
Comment on attachment 8970604 [details]
Bug 1456190 - 1. Add minimal RDP client for GV testing;

https://reviewboard.mozilla.org/r/239368/#review245116

Looks like a great start. I think some basic documentation for the major classes (Actor, Grip, RDPConnection) would help a lot, though.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rdp/RDPConnection.java:36
(Diff revision 1)
> +
> +    {
> +        mActors.put(mRoot.name, mRoot);
> +    }
> +
> +    public RDPConnection(final String name) {

path? fileName?
Attachment #8970604 - Flags: review?(snorp) → review+
Comment on attachment 8970605 [details]
Bug 1456190 - 2. Add evaluateJS API to GV test;

https://reviewboard.mozilla.org/r/239370/#review245120

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:841
(Diff revision 1)
>          if (sRuntime == null) {
>              final GeckoRuntimeSettings.Builder runtimeSettingsBuilder =
>                  new GeckoRuntimeSettings.Builder();
>              runtimeSettingsBuilder.arguments(new String[] { "-purgecaches" })
> -                                  .extras(InstrumentationRegistry.getArguments());
> +                                  .extras(InstrumentationRegistry.getArguments())
> +                                  .remoteDebuggingEnabled(true);

Should we use mWithDevTools instead of true?

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:888
(Diff revision 1)
> +                                                              .getApplicationInfo().dataDir;
> +                sRDPConnection = new RDPConnection(dataDir + "/firefox-debugger-socket");
> +                sRDPConnection.setTimeout((int) Math.min(getDefaultTimeoutMillis(),
> +                                                         Integer.MAX_VALUE));
> +            }
> +            final Tab tab = sRDPConnection.getMostRecentTab();

Can we do something more deterministic here? Get the tab by id (can we use the session id?)?
Attachment #8970605 - Flags: review?(snorp) → review+
Comment on attachment 8970606 [details]
Bug 1456190 - 3. Add tests for evaluateJS();

https://reviewboard.mozilla.org/r/239372/#review245122
Attachment #8970606 - Flags: review?(snorp) → review+
Flags: needinfo?(bugs) → needinfo?(pbrosset)
(In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> The part 1 patch adds a remote debugging protocol client to the GeckoView
> testing framework. It basically opens a socket to connect to devtools, and
> implements minimal support to get the actor for the active tab, and the
> webconsole actor under that. Finally, it implements the `evaluateJS` call
> and code for interacting with grips.
> 
> I think compatibility with devtools will be quite good given the minimal
> nature of this client. NI'ing Jet to keep the devtools team in the loop.
Thanks Jet for passing that along and Jim for keeping DevTools in the loop.
The DevTools protocol isn't extremely well documented at the moment nor versioned, and we make changes to it on a very regular basis. In the team, that's not much of a problem because we maintain both the client and server so we evolve them in sync. But there's some risk associated to creating a new client.
Having said that, if you only care about the evaluateJS console actor method, then I think we'll be really safe. That (as well as most of the js debugging actors) is very stable. We're not really changing any of this part of the protocol.

Let me needinfo Jim Blandy on this too so he's looped in too. And I'll cc Nicolas Chevobbe who's working on the Firefox web console front-end these days so he's aware too.
Flags: needinfo?(pbrosset) → needinfo?(jimb)
Comment on attachment 8970605 [details]
Bug 1456190 - 2. Add evaluateJS API to GV test;

https://reviewboard.mozilla.org/r/239370/#review245120

> Can we do something more deterministic here? Get the tab by id (can we use the session id?)?

It's pretty deterministic the way it is because we always get the tab right after creating it. But we can probably add a get-by-id thing down the road.
Attachment #8971631 - Attachment is obsolete: true
Attachment #8971631 - Flags: review?(snorp)
Should probably wait for bug 1456254 first before updating the new session tests.
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6bf3a14e2f9e
1. Add minimal RDP client for GV testing; r=snorp
https://hg.mozilla.org/integration/autoland/rev/6bac8d381551
2. Add evaluateJS API to GV test; r=snorp
https://hg.mozilla.org/integration/autoland/rev/3e1004a7729f
3. Add tests for evaluateJS(); r=snorp
Depends on: 1458276
This looks neat!

I agree with Patrick's comments about stability; evaluateJS should be fine, but the rest of it is not especially trustworthy.
Flags: needinfo?(jimb)
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.