1568327 - WebDriver - PerformActions should return InvalidArgument when passed an action sequence with an invalid id

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P3)

Description

[George Roman]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

I'm implementing actions support in Servo: https://github.com/servo/servo/pull/23805

Actual results:

No matter whether the id property exists or not, an Optional<String> is always returned:
https://searchfox.org/mozilla-central/source/testing/webdriver/src/command.rs#315
https://searchfox.org/mozilla-central/source/testing/webdriver/src/actions.rs#10

Expected results:

An InvalidArgument error should be returned when processing an action sequence with a missing or invalid id property.

Comment 1

[George Roman]

Based on the WebDriver spec: https://w3c.github.io/webdriver/#dfn-process-an-input-source-action-sequence

Comment 2

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

Interesting. Does removing the Option, and the Serde field attribute above fix it for you?

Comment 3

[George Roman]

Unfortunately I don't think that removing those fields would fix this as we need to return an InvalidArgument error when the id parameter is missing. Some additional check like in https://searchfox.org/mozilla-central/source/testing/webdriver/src/command.rs#306 would probably be needed.

Comment 4

[George Roman]

Attachment: Bug 1568327 - [geckodriver] Make ActionSequence's id field mandatory. r=ato,whimboo

Change ActionSequence's id field from Option<String> to String

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/facd6af4e16c
Make ActionSequence's id field mandatory r=whimboo

Comment 6

[Cristian Brindusan [:cbrindusan]]

Backed out changeset facd6af4e16c (Bug 1568327) for wpt failures at perform_actions/key.py.

Backout: https://hg.mozilla.org/integration/autoland/rev/fbe9eff3a3fcbb59a1da95c19037d98b72d21708

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=facd6af4e16c75b19417ae5e4ef5a906aced1fa4&selectedJob=259146320

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=259146320&repo=autoland&lineNumber=88966

Comment 7

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

I totally missed that the wdspec tests and/or the webdriver client might also need an update. As it looks like the id isn't yet or wrongly specified. The client can be found at: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/client.py

The wdspec tests can be run like:

$ mach wpt testing/web-platform/tests/webdriver/tests/perform_actions/ --webdriver-binary target/debug/geckodriver --webdriver-arg=-vv

Comment 8

[George Roman]

I updated the revision, added a test for this particular case and now the tests pass.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fff7b5299ecf
[geckodriver] Make ActionSequence's id field mandatory. r=whimboo

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18242 for changes under testing/web-platform/tests

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 12

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/fff7b5299ecf

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged