Closed
Bug 1456190
Opened 7 years ago
Closed 7 years ago
Implement a minimal remote debugging Java client for tests
Categories
(GeckoView :: General, enhancement, P2)
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
mozreview-review |
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 6•7 years ago
|
||
mozreview-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 7•7 years ago
|
||
mozreview-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+
Updated•7 years ago
|
Flags: needinfo?(bugs) → needinfo?(pbrosset)
Comment 8•7 years ago
|
||
(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)
Updated•7 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8971631 -
Attachment is obsolete: true
Attachment #8971631 -
Flags: review?(snorp)
Assignee | ||
Comment 17•7 years ago
|
||
Should probably wait for bug 1456254 first before updating the new session tests.
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6bf3a14e2f9e
https://hg.mozilla.org/mozilla-central/rev/6bac8d381551
https://hg.mozilla.org/mozilla-central/rev/3e1004a7729f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 20•7 years ago
|
||
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)
Updated•6 years ago
|
Product: Firefox for Android → GeckoView
Updated•6 years ago
|
Target Milestone: Firefox 61 → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•