Closed Bug 1507782 Opened 10 months ago Closed 4 months ago

Accept unknown JSON fields to WebDriver:SetTimeouts

Categories

(Testing :: Marionette, enhancement, P2)

enhancement

Tracking

(firefox-esr60 affected, firefox65 affected, firefox66 affected)

RESOLVED DUPLICATE of bug 1507283
Tracking Status
firefox-esr60 --- affected
firefox65 --- affected
firefox66 --- affected

People

(Reporter: ato, Unassigned)

Details

Attachments

(2 obsolete files)

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.
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?
Flags: needinfo?(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.
Flags: needinfo?(ato)

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?

  1. 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.

Blocks: 1507283
Flags: needinfo?(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.

Flags: needinfo?(ato)

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

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?

Flags: needinfo?(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.

Status: NEW → RESOLVED
Closed: 4 months ago
Flags: needinfo?(ato)
Resolution: --- → FIXED

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.

Fixes a lint problem.

This has actually ben fixed by bug 1507283.

Resolution: FIXED → DUPLICATE
Duplicate of bug: 1507283

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

Uggh, attached these to the wrong bug.

Attachment #9069342 - Attachment is obsolete: true
Attachment #9069343 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.