[webdriver] Test serialization of script values #13880
Conversation
There was a problem hiding this comment.
It seems the specification in this area is not in sync with implementations. For example, in addition to NodeList, HTMLCollection, and Array, we expect WebDriver to also serialise the following collection types:
- Arguments
- FileList
- HTMLAllCollection
- HTMLFormControlsCollection
- HTMLOptionsCollection
There are some tests for this in https://github.com/web-platform-tests/wpt/blob/master/webdriver/tests/execute_script/collections.py, which this PR partially duplicates in test_array_serialization and test_foreign_array_serialization. The other tests for primitives look absolutely fine.
We have two options: either we land this as it is, seeing it tests the actual wording in the spec, and fix up the tests later after addressing the missing collection types, or we take out the collection tests and land only the primitives tests.
Let me know what you’d prefer!
| # > Otherwise | ||
| # > A new Object. | ||
| # | ||
| # https://w3c.github.io/webdriver/#dfn-clone-an-object |
There was a problem hiding this comment.
Please leave out all quotes of the specification. We removed most of these some time ago because we weren’t able to keep them up with the changing spec.
| {"primitive": "false", "expected": False}, | ||
| {"primitive": "23", "expected": 23}, | ||
| {"primitive": "'a string'", "expected": "a string"} | ||
| ]) |
There was a problem hiding this comment.
nit: Instead of parametrizing on a dictionary with primitive and expected keys, I feel it would read better if you just exposed both values as arguments:
@pytest.mark.parametrize("primitive,expected", [("null", None), ("undefined", None), ("true", True), …])
def test_primitive_serialization(session, primitive, expected):
…
We also have a third: make sure we have rough consensus that the spec is wrong, and test what we want everyone to do. Obviously in a case like this, there's little reason not to also land the spec changes quickly (which @andreastt already has). |
|
Fair point @gsnedders. Seeing as w3c/webdriver#1340, let’s land tests that are to its intent. There are already collection tests in https://github.com/web-platform-tests/wpt/blob/master/webdriver/tests/execute_script/collections.py so I’m leaning towards dropping all the collection tests from this PR + addressing the parametrization nitpick. |
|
I'm happy to remove the redundant tests and use the more readable patterns for Pytest, but there's still some ambiguity about what is not a collection, so I'm going to hold off on further changes for now. |
|
@jugglinmike can we have at least one test that Edge fails for now? |
|
This patch was originally motivated by the need for tests of collection serialization. That's why I wanted to delay merging until the corresponding specification text was clear. However, I've just today come across a conformance issue in Edge/Edgedriver that concerns serialization of primitive values. In order to get that issue documented, I've limited the scope of this patch to primitive values (including a test case for Edge's bug). I'll use w3c/webdriver#1347 to continue the discussion about collections. @andreastt mind taking another look? |
|
@brendyna & @thejohnjansen The conformance issue I mentioned above is this: if a string value returned from "Execute Script" or "Execute Async Script" contains the null byte, Edgedriver truncates the string to the first occurrence of that character. |
|
Thanks for reporting that issue, @jugglinmike. @thejohnjansen and I will take a look and report back. Is the issue blocking this PR at all or will we simply pass a test once this bug is fixed on our end? |
|
Thanks for the quick response! The bug isn't blocking this pull request, though it may motivate a change to the testharness.js framework: gh-14245 |
|
@jugglinmike as it looks like this PR was forgotten. Do you still have interest to update it to the current state on master? |
|
Hey there, @whimboo! I have the interest but not the time. |
|
@jugglinmike thanks for the reply! Let me actually add those tests while I'm working on https://bugzilla.mozilla.org/show_bug.cgi?id=1819029. As such they will be upstreamed later this week. |
Tests originally written by jugglinmike: #13880 Differential Revision: https://phabricator.services.mozilla.com/D171234 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1819029 gecko-commit: f4cffe8e9381701473709db6054097d43396ce6e gecko-reviewers: webdriver-reviewers, jdescottes
…rimitive values. r=webdriver-reviewers,jdescottes Tests originally written by jugglinmike: web-platform-tests/wpt#13880 Differential Revision: https://phabricator.services.mozilla.com/D171234
Tests originally written by jugglinmike: #13880 Differential Revision: https://phabricator.services.mozilla.com/D171234 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1819029 gecko-commit: f4cffe8e9381701473709db6054097d43396ce6e gecko-reviewers: webdriver-reviewers, jdescottes
…rimitive values. r=webdriver-reviewers,jdescottes Tests originally written by jugglinmike: web-platform-tests/wpt#13880 Differential Revision: https://phabricator.services.mozilla.com/D171234
Tests originally written by jugglinmike: #13880 Differential Revision: https://phabricator.services.mozilla.com/D171234 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1819029 gecko-commit: f4cffe8e9381701473709db6054097d43396ce6e gecko-reviewers: webdriver-reviewers, jdescottes
No description provided.