1507782 - Accept unknown JSON fields to WebDriver:SetTimeouts

Status: RESOLVED DUPLICATE of bug 1507283

(Remote Protocol :: Marionette, enhancement, P2)

Description

[Andreas Tolfsen ❲:ato❳]

In addition to relaxing serde JSON deserialisation behavoiur (tracked
in https://bugzilla.mozilla.org/show_bug.cgi?id=1507283) we need
to make Marionette’s WebDriver:SetTimeouts command accept unknown
JSON fields.

As mentioned in
https://github.com/mozilla/geckodriver/issues/1422#issuecomment-439384695,
a specification change is also required for this change.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can you please explain why we need a separate bug for that? I thought that all can be done on bug 1507283.

Should we better make bug 1507283 a meta bug?

Comment 2

[Andreas Tolfsen ❲:ato❳]

See my comment here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1507283#c5

We have a literal implementation of the Set Timeouts deserialisation
description, erroring on unknown JSON fields, implemented in JS in
Marionette:
https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/testing/marionette/capabilities.js#62

That problem is orthogonal from the way I interpret
https://bugzilla.mozilla.org/show_bug.cgi?id=1507283, which is to
do with relaxing serde deserialisation in geckodriver.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so to wrap this up... You mean that we would have to change the following substep for deserialize as a timeout of the spec to just ignore those keys and not throwing an invalid argument error?

3. Find the timeout type from the table of session timeouts by looking it up by its keyword key.
   If no keyword matches key, return error with error code invalid argument.

If that is the case it will be a simple change, and I would like to see this uplifted to beta, and maybe even esr60.

I will mark bug 1507283 as being blocked by that change, so that I can verify the correct behavior of Firefox when making the geckodriver changes.

Comment 4

[Andreas Tolfsen ❲:ato❳]

Yes, the specification here is wrong too. But fixing just substep
three would be poor form. The whole algorithm can just be changed
to access the different timeout types directly. There’s no need
to iterate the object by own properties.

I also note with interest that substep 4 is wrong, because the
script timeout can be null:

If value is not an integer, or it is less than 0 or greater than
the maximum safe integer, return error with error code invalid
argument.

Looks like there’s a bit of spec work to get out of the way first.

Comment 5

[Andreas Tolfsen ❲:ato❳]

I rewrote the specification back in January, but forgot to post about it here:
https://github.com/w3c/webdriver/commit/b16bbbd420bcc763fa8d3dfba3dacdc1764cfb27

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andreas, did you also land some wdspec tests for those changes yet? I assume it will be a single test only for an unknown key?

Comment 7

[Andreas Tolfsen ❲:ato❳]

Actually, I think you did to a large degree in
https://bugzilla.mozilla.org/show_bug.cgi?id=1507283.

Judging by https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/set_timeouts/set.py
we’re still missing one test for an unknown key, as you said.

For the sake of clarity I’m resolving this and will follow up with
a new test in https://bugzilla.mozilla.org/show_bug.cgi?id=1556387.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1507782: webdriver: add test for ignored fields to Set Timeouts;

WebDriver is meant to ignore unfamiliar fields in JSON Objects generally,
and whilst we are testing that an empty object, {}, does not mutate
the timeout duration values, we are not testing unknown field keys.

Comment 9

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1507782: webdriver: compare None types with "is" operator;

Fixes a lint problem.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This has actually ben fixed by bug 1507283.

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9069342 [details]
bug 1507782: webdriver: add test for ignored fields to Set Timeouts;

Uggh, attached these to the wrong bug.