Closed
Bug 1461463
Opened 6 years ago
Closed 6 years ago
WebDriver commands which should return data "null" return an empty dictionary
Categories
(Remote Protocol :: Marionette, enhancement, P1)
Tracking
(firefox62 fixed)
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Blocks 1 open bug)
Details
Attachments
(7 files)
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
As lately seen while fixing some wdspec tests: > [task 2018-05-14T19:38:30.936Z] 19:38:30 INFO - 1526326710926 Marionette TRACE 151 -> [0,6,"WebDriver:AddCookie",{"cookie":{"path":"/","name":"foo","value":"bar","secure":false}}] > [task 2018-05-14T19:38:30.936Z] 19:38:30 INFO - 1526326710928 Marionette TRACE 151 <- [1,6,null,{}] The expected return value here is `null`, but an empty dictionary is returned. Here the spec entry: https://w3c.github.io/webdriver/#add-cookie > 8. Return success with data null. This applies to all commands which return `null` as value.
Comment 1•6 years ago
|
||
But this is not the return value from geckodriver, it is the return value from Marionette. Marionette does not match WebDriver 1:1 and I suspect you might find that geckodriver returns {"value": null}.
Assignee | ||
Comment 2•6 years ago
|
||
That's why I filed it in the Marionette component, and it may also need a patch for geckodriver if it doesn't pass-through the value 1:1. Here the log from a wdspec test job:
> [task 2018-05-15T05:48:15.015Z] 05:48:15 INFO - PID 1023 | 1526363295003 Marionette TRACE 0 -> [0,2541,"WebDriver:AddCookie",{"cookie":{"domain":"web-platform.test","httpOnly":false,"name":"hello","path":"/","secure":false,"value":"world"}}]
> [task 2018-05-15T05:48:15.017Z] 05:48:15 INFO - PID 1023 | 1526363295005 Marionette TRACE 0 <- [1,2541,null,{}]
> [task 2018-05-15T05:48:15.018Z] 05:48:15 INFO - PID 1023 | 1526363295006 webdriver::server DEBUG <- 200 OK {"value": {}}
Btw I talked with Jim yesterday and the IEDriver returns `null` in those cases.
Comment 3•6 years ago
|
||
Thanks for including the logs from webdriver::server, that was useful. Marionette returns {} (empty object) which geckodriver then wraps in {"value": <response body>}, so it looks like the value is just being passed through. Unless there is an edge case I don’t fully understand, I don’t see any particular value in changing the Marionette data format, but there’s probably a case to be made that it would be easier to change the return body to null so that geckodriver wouldn’t have to do any data massaging.
Assignee | ||
Comment 4•6 years ago
|
||
The problem originates from: https://dxr.mozilla.org/mozilla-central/rev/a382f8feaba41f1cb1cee718f8815cd672c10f3c/testing/marionette/message.js#239 > const ResponseBody = () => new Proxy({}, validator); The call to `Proxy` returns an object without any keys set, and if commands do not set a value we will just send this empty object over the socket. What we should do to send `null` is to check the body in `toPacket()` for contained keys. If there are none we should just send `null`.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P1
Assignee | ||
Comment 5•6 years ago
|
||
The change as proposed above works fine with Marionette and by running a couple of test modules I cannot find any failure. So it shouldn't be a backward incompatible change.
Also I checked the fix with wdspec and geckodriver, and as it looks like we already do some massaging of the response value:
> 0:19.55 pid:18803 1526388612895 Marionette TRACE 0 -> [0,127,"WebDriver:SwitchToWindow",{"name":"8589934593"}]
> 0:19.55 pid:18803 1526388612899 Marionette TRACE 0 <- [1,127,null,null]
> 0:19.56 pid:18803 1526388612899 webdriver::server DEBUG <- 200 OK {"value": {}}
So we clearly also need a fix in geckodriver, which can handle both response formats from Marionette.
Assignee | ||
Comment 6•6 years ago
|
||
The convertion from None to `{}` in webdriver rust happens here: https://dxr.mozilla.org/mozilla-central/rev/a382f8feaba41f1cb1cee718f8815cd672c10f3c/testing/webdriver/src/response.rs#25-36 I cannot figure out how to best solve this given that obj is expected to be a String, and None isn't nor have a string representation. Would we have to re-write this whole method? Andreas, maybe you could give an advise? Thanks.
Flags: needinfo?(ato)
Comment 7•6 years ago
|
||
This is a rather stupid serialiser. Just s/{}/null/g should work.
Flags: needinfo?(ato)
Assignee | ||
Comment 8•6 years ago
|
||
Oh, that easy! It works, and I will update the wdspec tests with additional assertions to check that those commands which should not return a value really return `null`.
Assignee | ||
Comment 9•6 years ago
|
||
Hm, testing for `None` in `assert_success` doesn't work: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/support/asserts.py#85-86 So unless we won't require an always assert for the value situation, we might have to assert separately by the return value in the test. Andreas, do you have another idea?
Flags: needinfo?(ato)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(ato)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8976537 -
Flags: review?(ato)
Attachment #8976538 -
Flags: review?(ato)
Attachment #8976539 -
Flags: review?(ato)
Attachment #8976540 -
Flags: review?(ato)
Attachment #8976541 -
Flags: review?(ato)
Assignee | ||
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8976537 [details] Bug 1461463 - [marionette] Empty response value should be null and not {}. https://reviewboard.mozilla.org/r/244646/#review250838 ::: testing/marionette/driver.js:1193 (Diff revision 2) > "Marionette:waitForPageLoaded", parameters); > }); > > await goBack; > + > + return null; Any idea why eslint complains about this addition here with https://eslint.org/docs/rules/no-return-await?
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8976537 [details] Bug 1461463 - [marionette] Empty response value should be null and not {}. https://reviewboard.mozilla.org/r/244646/#review250974 The return value from a function which does not specify one is undefined, and the this would normally get serialised to null. For this reason I don’t think it’s the right solution to explicitly add "return null" to every command, but to rather fix the underlying problem which is the _default response body value_. You will find the details in testing/marionette/message.js’s Response class. ::: commit-message-87cc1:3 (Diff revision 2) > +Bug 1461463 - [marionette] Empty response value should be "null" and not "{}". > + > +WebDriver commands which do not return a value have to send "null'. s/"// s/'// It’s a null type, not a string. ::: commit-message-87cc1:5 (Diff revision 2) > +the spec we should make that change even it wouldn't be necessary > +due to geckodriver translates it again. This last part I don’t understand.
Attachment #8976537 -
Flags: review?(ato) → review-
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8976538 [details] Bug 1461463 - [geckodriver] Remove extra colon from invalid "WebDriver::Forward" command. https://reviewboard.mozilla.org/r/244648/#review250980 ::: commit-message-aacd5:1 (Diff revision 2) > +Bug 1461463 - [geckodriver] Fix WebDriver:Forward command. This commit message is not adequate.
Attachment #8976538 -
Flags: review?(ato) → review-
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8976539 [details] Bug 1461463 - [geckodriver] Empty response value should be null and not {}. https://reviewboard.mozilla.org/r/244650/#review250982 ::: commit-message-de840:1 (Diff revision 2) > +Bug 1461463 - [geckodriver] Empty response value should be "null" and not "{}". These are not strings but types, please remove quote marks. ::: commit-message-de840:3 (Diff revision 2) > +Bug 1461463 - [geckodriver] Empty response value should be "null" and not "{}". > + > +WebDriver commands which do not return a value have to send "null'. Remove quote marks.
Attachment #8976539 -
Flags: review?(ato) → review-
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8976540 [details] Bug 1461463 - [wdspec] Add timeout reset finalizer to session fixture. https://reviewboard.mozilla.org/r/244652/#review250984 ::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:178 (Diff revision 2) > if not _current_session.session_id: > raise > > # finalisers are popped off a stack, > # making their ordering reverse > + request.addfinalizer(lambda: _restore_timeouts(_current_session)) If the ordering is reverse, I would suggest setting this first since some of the other finalisers are interacting with WebDriver in such a way that they might be influenced by the timeout duration values.
Attachment #8976540 -
Flags: review?(ato) → review+
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8976541 [details] Bug 1461463 - [wdspec] Add tests for commands returning null as response value. https://reviewboard.mozilla.org/r/244654/#review250986 This is an excellent patch. Thanks for doing this due diligence. ::: commit-message-8a3f2:1 (Diff revision 2) > +Bug 1461463 - [wdspec] Add tests for commands returning "null" as response value. null is a type, not a string. Remove quote marks. ::: testing/web-platform/tests/webdriver/tests/accept_alert/accept.py:14 (Diff revision 2) > + value = assert_success(response) > + assert value is None Throughout this would be equivalent to assert_success(response, None), but this is obviously fine. ::: testing/web-platform/tests/webdriver/tests/actions/key.py:7 (Diff revision 2) > +def test_null_response_value(session, key_chain): > + value = key_chain.key_up("a").perform() > + assert value is None > + > + value = session.actions.release() > + assert value is None I don’t know if there’s a particular value in testing mouse/key chains separately since these are essentially the same endpoints? ::: testing/web-platform/tests/webdriver/tests/back/back.py:11 (Diff revision 2) > + return session.transport.send( > + "POST", "session/{session_id}/back".format(**vars(session))) > + > + > +def test_null_response_value(session): > + session.url = inline("<div/>") BTW this doesn’t actually close the element AFAIK, but obviously not an issue. ::: testing/web-platform/tests/webdriver/tests/delete_cookie/delete.py:12 (Diff revision 2) > session_id=session.session_id, > name=name)) > > > +def test_null_response_value(session, url): > + response = delete_cookie(session, "foo") Does the cookie already exist somehow, or does it always return success?
Attachment #8976541 -
Flags: review?(ato) → review+
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8976538 [details] Bug 1461463 - [geckodriver] Remove extra colon from invalid "WebDriver::Forward" command. https://reviewboard.mozilla.org/r/244648/#review250988
Attachment #8976538 -
Flags: review- → review+
Assignee | ||
Comment 27•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8976537 [details] Bug 1461463 - [marionette] Empty response value should be null and not {}. https://reviewboard.mozilla.org/r/244646/#review250974 > This last part I don’t understand. Maybe I should drop? The Marionette socket protocol doesn't have to obey the WebDriver specification, and as such it might not be necessary to return null. But it helps geckodriver to more generally return null for those commands which require it. Otherwise we would have to add code for each Response type in webdriver which convers "{}" to null.
Assignee | ||
Comment 28•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8976540 [details] Bug 1461463 - [wdspec] Add timeout reset finalizer to session fixture. https://reviewboard.mozilla.org/r/244652/#review250984 > If the ordering is reverse, I would suggest setting this first since > some of the other finalisers are interacting with WebDriver in such > a way that they might be influenced by the timeout duration values. Good idea. Will do it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•6 years ago
|
||
I discussed with Andreas how we could improve the situation in Marionette with handling the return value. We identified a couple of solutions which I will test later today if those work. I will comment then in more detail. For now I removed the review request from this particular commit.
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8976539 [details] Bug 1461463 - [geckodriver] Empty response value should be null and not {}. https://reviewboard.mozilla.org/r/244650/#review251042
Attachment #8976539 -
Flags: review?(ato) → review+
Comment 36•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8976537 [details] Bug 1461463 - [marionette] Empty response value should be null and not {}. https://reviewboard.mozilla.org/r/244646/#review250974 > Maybe I should drop? The Marionette socket protocol doesn't have to obey the WebDriver specification, and as such it might not be necessary to return null. But it helps geckodriver to more generally return null for those commands which require it. Otherwise we would have to add code for each Response type in webdriver which convers "{}" to null. Indeed, I agree with what you just said here.
Assignee | ||
Comment 37•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8976537 [details] Bug 1461463 - [marionette] Empty response value should be null and not {}. https://reviewboard.mozilla.org/r/244646/#review250974 > Indeed, I agree with what you just said here. So does it mean I have to update that part of the commit message, or drop it?
Comment 38•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8976537 [details] Bug 1461463 - [marionette] Empty response value should be null and not {}. https://reviewboard.mozilla.org/r/244646/#review250974 > So does it mean I have to update that part of the commit message, or drop it? Dropping it makes sense, since you say it doesn’t have a direct relation to WebDriver.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 64•6 years ago
|
||
mozreview-review |
Comment on attachment 8980339 [details] Bug 1461463 - [marionette] Add validation for known contexts. https://reviewboard.mozilla.org/r/246492/#review252880 Whilst I don’t disagree that having a setter for this.context is probably somewhat safer that the status quo, you appear to have misunderstood what the Context type is in the code in the setter. Context.Content and Context.Chrome are both primitive string types, which means the setter is far more complicated than it needs to be. The "context in [Context.Content, Context.Chrome]" check will always pass (given valid input) and the type check on :173 will never be called. :176 also duplicates code in Context.fromString.
Attachment #8980339 -
Flags: review?(ato) → review-
Comment 65•6 years ago
|
||
mozreview-review |
Comment on attachment 8980340 [details] Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional. https://reviewboard.mozilla.org/r/246494/#review252884 ::: commit-message-59d5b:1 (Diff revision 4) > +Bug 1461463 - [wdclient] Raising SessionNotCreatedException requires message. I’m pretty sure it doesn’t. What problem are you trying to fix here?
Attachment #8980340 -
Flags: review?(ato) → review-
Comment 66•6 years ago
|
||
mozreview-review |
Comment on attachment 8976537 [details] Bug 1461463 - [marionette] Empty response value should be null and not {}. https://reviewboard.mozilla.org/r/244646/#review252888
Attachment #8976537 -
Flags: review?(ato) → review+
Assignee | ||
Comment 67•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8980340 [details] Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional. https://reviewboard.mozilla.org/r/246494/#review252884 > I’m pretty sure it doesn’t. What problem are you trying to fix > here? It needs a message otherwise we see an exception in this line. The message is not optional for WebDriverException which SessionNotCreatedException is based on: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/error.py#112 https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/error.py#9 You can replicate that when ending the session, and calling a session command like we do in this patch series for restoring the timeouts.
Assignee | ||
Comment 68•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8980339 [details] Bug 1461463 - [marionette] Add validation for known contexts. https://reviewboard.mozilla.org/r/246492/#review252880 So I should better remove that commit then?
Assignee | ||
Comment 69•6 years ago
|
||
Comment on attachment 8976537 [details] Bug 1461463 - [marionette] Empty response value should be null and not {}. Please review the latest changes again.
Attachment #8976537 -
Flags: review?(ato)
Assignee | ||
Comment 70•6 years ago
|
||
Comment on attachment 8976537 [details] Bug 1461463 - [marionette] Empty response value should be null and not {}. Sorry, looks like there was an overlap. All fine.
Attachment #8976537 -
Flags: review?(ato)
Comment 71•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8980340 [details] Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional. https://reviewboard.mozilla.org/r/246494/#review252884 > It needs a message otherwise we see an exception in this line. > > The message is not optional for WebDriverException which SessionNotCreatedException is based on: > https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/error.py#112 > https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/error.py#9 > > You can replicate that when ending the session, and calling a session command like we do in this patch series for restoring the timeouts. Ugh. It feels like the right thing to do is to make message=None so that we don’t have to pass an argument, because "No active session" is basically duplicating the name of the exception type.
Comment 72•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8980339 [details] Bug 1461463 - [marionette] Add validation for known contexts. https://reviewboard.mozilla.org/r/246492/#review252880 As I said, I think it’s a good idea to have a setter for it that checks the input is correct every time it is called. It makes the code marginally safer.
Assignee | ||
Comment 73•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8980340 [details] Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional. https://reviewboard.mozilla.org/r/246494/#review252884 > Ugh. It feels like the right thing to do is to make message=None > so that we don’t have to pass an argument, because "No active > session" is basically duplicating the name of the exception type. This change will be part of my next update.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 81•6 years ago
|
||
mozreview-review |
Comment on attachment 8980339 [details] Bug 1461463 - [marionette] Add validation for known contexts. https://reviewboard.mozilla.org/r/246492/#review253572
Attachment #8980339 -
Flags: review?(ato) → review+
Comment 82•6 years ago
|
||
mozreview-review |
Comment on attachment 8980340 [details] Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional. https://reviewboard.mozilla.org/r/246494/#review253574 ::: testing/web-platform/tests/tools/webdriver/webdriver/error.py:20 (Diff revision 5) > + if self.message is not None: > + message += ": %s\n" % self.message > + else: > + message += "\n" Looks like the new line must be added regardless, so the else-clause is not necessary.
Attachment #8980340 -
Flags: review?(ato) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 90•6 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5820f713fe9a [marionette] Add validation for known contexts. r=ato https://hg.mozilla.org/integration/autoland/rev/2df57c387242 [marionette] Empty response value should be null and not {}. r=ato https://hg.mozilla.org/integration/autoland/rev/1c4a5bba3582 [geckodriver] Remove extra colon from invalid "WebDriver::Forward" command. r=ato https://hg.mozilla.org/integration/autoland/rev/9d1398fa66ba [geckodriver] Empty response value should be null and not {}. r=ato https://hg.mozilla.org/integration/autoland/rev/abd4d105082a [wdclient] "message" argument of WebDriverException has to be optional. r=ato https://hg.mozilla.org/integration/autoland/rev/f4261bd006df [wdspec] Add timeout reset finalizer to session fixture. r=ato https://hg.mozilla.org/integration/autoland/rev/fe8aa70475dd [wdspec] Add tests for commands returning null as response value. r=ato
Comment 91•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5820f713fe9a https://hg.mozilla.org/mozilla-central/rev/2df57c387242 https://hg.mozilla.org/mozilla-central/rev/1c4a5bba3582 https://hg.mozilla.org/mozilla-central/rev/9d1398fa66ba https://hg.mozilla.org/mozilla-central/rev/abd4d105082a https://hg.mozilla.org/mozilla-central/rev/f4261bd006df https://hg.mozilla.org/mozilla-central/rev/fe8aa70475dd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11953 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/11953 * continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/403139652?utm_source=github_status&utm_medium=notification)
Upstream PR merged
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•