Closed Bug 807282 Opened 7 years ago Closed 6 years ago

Update Marionette's command/response to match that of the W3C spec for easier interop with WebDriver

Categories

(Testing :: Marionette, defect)

x86
macOS
defect
Not set

Tracking

(firefox25 wontfix, firefox26 fixed, firefox27 fixed)

RESOLVED FIXED
mozilla27
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: automatedtester, Assigned: automatedtester)

References

(Depends on 1 open bug, )

Details

Attachments

(1 file, 3 obsolete files)

The command/response structure that Marionette uses does not match WebDriver and the W3C spec currently we have 
{
    "sessionId" : <sessionId>,
    "name" : <commandName>,
    "param1" : "foo",
    "param2" : "bar"
}

Where we should have

{
    "sessionId" : <sessionId>,
    "name" : <commandName>,
    "parameters" :
    {
        // parameters name/value pairs go here.
    }
}

See spec details for more information
Blocks: 797912
We're actually using 'type' as the 'name' field, and it's now breaking our ability to match w3c spec of some methods! One in particular is 'timeouts': https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#timeouts-1


One problem is that we're using the debugger server stuff, so it expects a packet with 'type' as the requestType, which is what we use to find the method on the serverside. I think this bug will have to be addressed once we rip out the debugger server from marionette (Bug 797529)
Depends on: 797529
Assignee: nobody → dburns
Depends on: 819050
*sigh* after going through the pain of updating my VM with B2G I have solved the failing try on b2g (I think)


https://tbpl.mozilla.org/?tree=Try&rev=21fc05d028a4
https://tbpl.mozilla.org/?tree=Try&rev=0020a1431e6e
Flags: needinfo?(dburns)
try looks good for desktop. try has bug 819050 patch with it as they will probably need landing at the same time.

https://tbpl.mozilla.org/?tree=Try&rev=acf9917416c2

Mobile submitted at 
https://tbpl.mozilla.org/?tree=Try&rev=8fafef651570
Flags: needinfo?(dburns)
Attachment #811321 - Attachment is obsolete: true
Attachment #811332 - Flags: review?(mdas)
Comment on attachment 811332 [details] [diff] [review]
Update marionette command/response to match that of the WebDriver spec;

Review of attachment 811332 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for doing this! I just have one small nit, but otherwise, this is great.

::: testing/marionette/marionette-server.js
@@ +603,2 @@
>      if (context != "content" && context != "chrome") {
> +      this.sendError("invalid context. Context sent was", 500, null, this.command_id);

The error message is incomplete, it doesn't tell you what the context set was:)
Attachment #811332 - Flags: review?(mdas) → review+
Comment on attachment 811332 [details] [diff] [review]
Update marionette command/response to match that of the WebDriver spec;

Review of attachment 811332 [details] [diff] [review]:
-----------------------------------------------------------------

I just realized that the docstrings should be updated with the correct parameter information, please! I just highlighted a few examples.

::: testing/marionette/marionette-server.js
@@ +712,2 @@
>     * @param object aRequest
>     *        'value' member is the script to run

Change this to 'script'

@@ +1027,5 @@
>    /**
>     * Navigates to given url
>     *
>     * @param object aRequest
>     *        'value' member holds the url to navigate to

change this to 'url'
Attachment #811332 - Attachment is obsolete: true
Comment on attachment 812664 [details] [diff] [review]
Update marionette command/response to match that of the WebDriver spec;

Review of attachment 812664 [details] [diff] [review]:
-----------------------------------------------------------------

Updated docs after review. carrying r+ forward
Attachment #812664 - Flags: review+
Attached patch bug807282.patchSplinter Review
rebased patch attached, carrying r+ forward
Attachment #812664 - Attachment is obsolete: true
Attachment #817091 - Flags: review+
Summary: Update Marionettes command/response to match that of the W3C spec for easier interop with WebDriver → Update Marionette's command/response to match that of the W3C spec for easier interop with WebDriver
https://hg.mozilla.org/mozilla-central/rev/65d2859ad74a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(In reply to Jonathan Griffin (:jgriffin) from comment #14)
> pushed to pypi: https://pypi.python.org/pypi/marionette_client/0.6.0

Jonathan -- did this break us?

http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.xfail/21/console

21:47:17 Best match: marionette-client 0.6.0
21:47:17 Downloading https://pypi.python.org/packages/source/m/marionette_client/marionette_client-0.6.0.tar.gz#md5=1cee54cbe126dc41e2bf8b1a618eb1fa
21:47:17 Processing marionette_client-0.6.0.tar.gz
21:47:17 Running marionette_client-0.6.0/setup.py -q bdist_egg --dist-dir /tmp/easy_install-ArS5Ms/marionette_client-0.6.0/egg-dist-tmp-vQRkwL
21:47:17 Adding marionette-client 0.6.0 to easy-install.pth file
21:47:17 Installing marionette script to /var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.xfail/.env/bin

...
21:47:25   File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.xfail/.env/local/lib/python2.7/site-packages/marionette_client-0.6.0-py2.7.egg/marionette/client.py", line 79, in connect
21:47:25     self.actor = response['id']
21:47:25 KeyError: 'id'

http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.xfail/19/console seemed to be fine.
(In reply to Stephen Donner [:stephend] from comment #15)
> (In reply to Jonathan Griffin (:jgriffin) from comment #14)
> > pushed to pypi: https://pypi.python.org/pypi/marionette_client/0.6.0
> 
> Jonathan -- did this break us?

Yes. I believe we're dependent on new nightly builds. For the perf tests I have temporarily pinned them to marionette client 0.5.37.
Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/896009f4bac5 to find out whether it was you or bug 779284 which turned b2g mochitest-7 permaorange without it bothering to say why it was orange, a la https://tbpl.mozilla.org/php/getParsedLog.php?id=29217047&tree=Mozilla-Aurora. Come on back in if it turns out not to be you.
This is quite strange, but this patch caused mochitest-7 on aurora to fail, for no apparent reason.  Will have to investigate before attempting to turn back on.
The failure is caused by threading output:

15:52:30     INFO -  1967 INFO TEST-START | Shutdown
15:52:30     INFO -  1968 INFO Passed:  1176
15:53:38     INFO -  1969 INFO FailedMochitest INFO | runtestsb2g.py | Running tests: end.
15:53:41     INFO -  :  0
15:53:41     INFO -  1970 INFO Todo:    1
15:53:41     INFO -  1971 INFO Slowest: 3948ms - /tests/dom/tests/mochitest/dom-level2-html/test_HTMLAreaElement04.html
15:53:41     INFO -  1972 INFO SimpleTest FINISHED

Here, our regex's can't determine the number of failed tests, because the output is interleaved with a notification from the runner.

It's really unclear why this particular patch would tickle this problem, although it doesn't directly cause it.
Depends on: 928034
Because there's no try access for aurora, I've re-landed this with a small hack to runtestsb2g.py which should prevent this problem:

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce79da6bb7ca
As has become my habit while eating dinner of late, backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/2664ac296982 for turning b2g mochitest-7 permaorange without it bothering to say why it was orange, a la https://tbpl.mozilla.org/php/getParsedLog.php?id=29283849&tree=Mozilla-Aurora
Life is going to be rough (and is rough as it is with other breaking changes between 1.2 and 1.3) without having this on Aurora.

Who can figure this out the quickest so we can get this landed?
Flags: needinfo?(dburns)
Yes we (webqa) are completely blocked on our Aurora/device testing by this :(
This really isn't caused by AutomatedTester's patch; it's a bug in mozprocess, apparently, that this patch just happens to tickle.  I'm working on it.
Flags: needinfo?(dburns)
(In reply to Jonathan Griffin (:jgriffin) from comment #26)
> try again: https://hg.mozilla.org/releases/mozilla-aurora/rev/747145639df6

mochitest-7 is green, so it looks like this is sticking this time :)
You need to log in before you can comment on or make changes to this bug.