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

Refactor RemoteValue serialization / deserialization tests #37160

Open whimboo opened 1 year ago

whimboo commented 1 year ago

With the current state of the WebDriver BiDi specification we have three different commands / events that will / could return a RemoteValue in their payload. These are the commands script.evaluate and script.callFunction, and the event log.entryAdded.

Excessive tests for serialization and deserialization can be found for both commands and which are mainly duplicated:

serialization: https://github.com/web-platform-tests/wpt/blob/master/webdriver/tests/bidi/script/evaluate/result.py https://github.com/web-platform-tests/wpt/blob/master/webdriver/tests/bidi/script/call_function/result.py https://github.com/web-platform-tests/wpt/blob/master/webdriver/tests/bidi/log/entry_added/console_args.py

deserialization: https://github.com/web-platform-tests/wpt/blob/master/webdriver/tests/bidi/script/call_function/arguments.py

In the future there might be potential other commands / events as well, which would require serialization / deserialization.

To prevent duplication of all the various scenarios with the caveat of increased maintenance and execution time per CI job we should try to centralize all the excessive tests and let all various commands only perform a subset of tests to ensure that the serialization / deserialization code path is correctly triggered.

My suggestion would be to create a new top-level folder under /webdriver/tests/bidi/remote_value and add all the excessive tests there by using script.callFunction. I'm going to propose that method due to its flexibility and because it requires both the serialization (return value) and deserialization (arguments). Identical tests can then be removed from other commands (script.evaluate) and be replaced by simple validation checks.

I'm going to propose this topic on our agenda for the W3C meeting. If anyone has feedback before next Thursday please feel free to already add your comment here. Thanks.

CC'ing @sadym-chromium, @juliandescottes, @jgraham, @lutien, @foolip.

sadym-chromium commented 1 year ago

Other option is to merge all the serialiation tests, and verify serialization of each type in different ways in one test. It would decrease atomicity of the tests, but would allow to keep the high coverage with relatively low runtime, as most of the time is spent on setting the test up. Deserialization is happening only in 1 place (script.callFunction), so it's not an issue.

Eg the test would look like:

@pytest.mark.parametrize("jsString, excepted_serialized", [("undefined", { "type": "undefined" }), ("null", { "type": "null" })])
test_serialization(...)
  assert evaluate(jsString) == excepted_serialized
  assert callFunction(jsString) == excepted_serialized
  assert throwException(jsString) == excepted_serialized
  ...  
whimboo commented 1 year ago

I don't think that this would lower the complexity and maintenance. Different commands / events would / might require other pre-conditions to be setup, and for each of them we would have to add new lines of code to every test. Note that this will be similar to session.subscribe and session.unsubscribe where we won't be able to test all the scenarios as well.

whimboo commented 1 year ago

This topic has been discussed yesterday and notes can be found at: https://www.w3.org/2022/12/01-webdriver-minutes.html#t04