web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
4.88k stars 3.05k forks source link

Explicitly check if parameters are mandatory in webdriver bidi commands #34697

Open juliandescottes opened 2 years ago

juliandescottes commented 2 years ago

We are checking most WebDriver bidi command arguments in invalid.py test files, but we don't have an easy and consistent way of checking if a parameter is mandatory.

For instance in https://github.com/web-platform-tests/wpt/pull/34577/files#diff-197193c766cc57eb1df9075f55dcbe488788e934a0fc9ba319f6db32c96d7eddR70 we added "None" to the list of invalid values for await_promise. But the only thing that this actually checks is that "null" is not a valid value, because at the same time we made await_promise mandatory in the test module script.py.

The test would have worked if we had kept await_promise optional in the test script.py module, because in this case we use "None" as a way to omit the parameter. Example from the old version of the script module:

if await_promise is not None:
    params["await_promise"] = await_promise

But we can't really use None to mean both null and undefined. Since there's no notion of undefined in pyton, maybe we need an undef fixture which we can use to really indicate that a parameter should be omitted?

cc @sadym-chromium

whimboo commented 2 years ago

That is a good question! In WebDriver classic tests we do not actually use the client's API for specific commands but re-implement the logic on top of the transport layer:

https://github.com/web-platform-tests/wpt/blob/master/webdriver/tests/execute_script/__init__.py

Not sure if we should follow this method at least for these kind of edge cases. Not being able to test mandatory vs optional arguments and how these are handled by the remote end would be a big loss.

One other option would be to mark all arguments as optional in the BiDi client, and always let the remote end handle missing arguments.

Also cc @jgraham.

juliandescottes commented 2 years ago

One other option would be to mark all arguments as optional in the BiDi client

Good point, that would work. All optional with no default value would fully rely on the remote end to perform the validation.

Although, if we keep the current approach (comparing to None to remove the parameter), there is a potential issue if we want to pass "null" explicitly to a command.

whimboo commented 2 years ago

Maybe we should consider to start using custom classes on the local end for all the types that are supported by WebDriver BiDi? Similar to:

class Null:
    def __init__(self):
        self.type = "null"

class Undefined:
    def __init__(self):
        self.type = "undefined"

class Number:
    ...

With the correct JSON serialization / deserialization on the transport layer we could then make use of eg. Null to pass in a BiDi null value. It would also help with our tests and not having that much boilerplate. But it might take a bit of work.

sadym-chromium commented 2 years ago

Maybe we should consider to start using custom classes on the local end for all the types that are supported by WebDriver BiDi? Similar to:

class Null:
    def __init__(self):
        self.type = "null"

class Undefined:
    def __init__(self):
        self.type = "undefined"

class Number:
    ...

With the correct JSON serialization / deserialization on the transport layer we could then make use of eg. Null to pass in a BiDi null value. It would also help with our tests and not having that much boilerplate. But it might take a bit of work.

It seems to be the proper solution

whimboo commented 10 months ago

@jrandolf I assume we have to reopen and it now depends on #42913?