Closed Bug 1453009 Opened 6 years ago Closed 6 years ago

Port WebDriver:ExecuteScript collection tests to WPT

Categories

(Testing :: geckodriver, enhancement, P2)

Product:
Testing
Component:
geckodriver
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1453009 - Test serialisation of collections in wdspec.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1453009 - Remove collection tests from Marionette.
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
Marionette has a number of good tests for collections that WebDriver
is supposed to serialise when returned from WebDriver:ExecuteScript
and WebDriver:ExecuteAsyncScript.  We should port these to WPT.

These are tests for the following collections:

  - Arguments
  - Array
  - FileList
  - HTMLAllCollection
  - HTMLCollection
  - HTMLFormControlsCollection
  - HTMLOptionsCollection
  - NodeList
With WPT I assume you mean wdspec tests?
Component: Marionette → geckodriver
Blocks: 1453057
Blocks: 1447977
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P2
Comment on attachment 8966663 [details]
Bug 1453009 - Add assert_element to wdspec.

https://reviewboard.mozilla.org/r/235362/#review241284
Attachment #8966663 - Flags: review?(hskupin) → review+
Comment on attachment 8966664 [details]
Bug 1453009 - Test serialisation of collections in wdspec.

https://reviewboard.mozilla.org/r/235364/#review241290

Can you please make it more clear in the name of the test module what it is about? It's for correct serialization of returned collections. So maybe `response_collections`? I didn't add comments for the `execute_script` changes, because it would be identical.

::: testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py:27
(Diff revision 2)
> +        args = []
> +    body = {"script": script, "args": args}
> +    return session.transport.send(
> +        "POST",
> +        "/session/{session_id}/execute/async".format(
> +            session_id=session.session_id),

This can simply be: `format(**vars(session))`.

::: testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py:55
(Diff revision 2)
> +    ps = session.find.css("p")
> +
> +    response = execute_async_script(session, """
> +        let [resolve] = arguments;
> +        let ps = document.querySelectorAll("p");
> +        resolve(Array.from(ps));

So if we fail to serialize a HTMLElement, this test would fail inappropriately, because it is not related to some broken behavior in serializing an array. Why don't you return something simple here?

::: testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py:61
(Diff revision 2)
> +        """)
> +    value = assert_success(response)
> +    assert isinstance(value, list)
> +    assert len(value) == 2
> +    for expected, actual in zip(ps, value):
> +        assert_element(actual)

hm, do we really need `assert_element`? Why can't it be part of `assert_same_element`?

::: testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py:70
(Diff revision 2)
> +def test_file_list(session, tempfile):
> +    files = [tempfile, tempfile]
> +
> +    session.url = inline("<input type=file>")
> +    upload = session.find.css("input", all=False)
> +    for file in files:

Note that you have the same file twice, and that your input is not using the `multiple` attribute. As such the filelist has only a single file.

I assume that the broken behavior due to bug 1448792 makes this test possible.

::: testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py:88
(Diff revision 2)
> +        assert isinstance(actual["name"], basestring)
> +        assert os.path.basename(str(expected)) == actual["name"]
> +
> +
> +
> +def test_html_all_collection(session):

nit: too many empty lines the linter should complain about.

::: testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py:135
(Diff revision 2)
> +    for expected, actual in zip(ps, value):
> +        assert_element(actual)
> +        assert_same_element(session, expected, actual)
> +
> +
> +def test_html_form_controls_collection(session):

What makes that and other special sub tests different from `test_html_collection` when we don't check for specific attributes and properties? I doubt we can/want to test all element types.
Attachment #8966664 - Flags: review?(hskupin) → review-
Comment on attachment 8966698 [details]
Bug 1453009 - Remove collection tests from Marionette.

https://reviewboard.mozilla.org/r/235404/#review241302
Attachment #8966698 - Flags: review?(hskupin) → review+
Comment on attachment 8966664 [details]
Bug 1453009 - Test serialisation of collections in wdspec.

https://reviewboard.mozilla.org/r/235364/#review241290

I don’t think that name is better.

> So if we fail to serialize a HTMLElement, this test would fail inappropriately, because it is not related to some broken behavior in serializing an array. Why don't you return something simple here?

So at some level you have to just trust other parts of the machinery
to work.  We’re not testing web element serialisation here, but if
that was broken I would expect the test to fail inappropriately.

> nit: too many empty lines the linter should complain about.

I think the linter will only complain about too _few_ lines.

> What makes that and other special sub tests different from `test_html_collection` when we don't check for specific attributes and properties? I doubt we can/want to test all element types.

I don’t I think I understand what you’re trying to say here.  We’re
testing all the special collection types that matter, from NodeList
to HTMLAllCollection to HTMLFormControlsCollection.

In this particular test, document.forms[N].elements returns a
HTMLFormControlsCollection which is distinct from, say, NodeList
which is normally returned from element looks through
document.getElementsByTagName and document.querySelectorAll.
Attachment #8966663 - Attachment is obsolete: true
Comment on attachment 8966664 [details]
Bug 1453009 - Test serialisation of collections in wdspec.

https://reviewboard.mozilla.org/r/235364/#review241290

Just an example. I'm sure you can find something better.

> So at some level you have to just trust other parts of the machinery
> to work.  We’re not testing web element serialisation here, but if
> that was broken I would expect the test to fail inappropriately.

Note that it might be harder then to find the underlying issue because the response covered a much broader list of collection types. Just returning a list of known to be serializable types, would be better IMO.
Comment on attachment 8966664 [details]
Bug 1453009 - Test serialisation of collections in wdspec.

https://reviewboard.mozilla.org/r/235364/#review241290

If you don’t mind I will stick with the original name.

> Note that it might be harder then to find the underlying issue because the response covered a much broader list of collection types. Just returning a list of known to be serializable types, would be better IMO.

I don’t think you can replace the contents of HTMLAllCollection or
any of the other collections with something that is known to be
serialisable, e.g. a primitive.

Or do you mean just this specific test for arrays?
Comment on attachment 8966664 [details]
Bug 1453009 - Test serialisation of collections in wdspec.

https://reviewboard.mozilla.org/r/235364/#review241290

Then leave it.

> I don’t think you can replace the contents of HTMLAllCollection or
> any of the other collections with something that is known to be
> serialisable, e.g. a primitive.
> 
> Or do you mean just this specific test for arrays?

Correct. Something like `respond(Array(1, 2, 3))`.
Comment on attachment 8966664 [details]
Bug 1453009 - Test serialisation of collections in wdspec.

https://reviewboard.mozilla.org/r/235364/#review241290

> Correct. Something like `respond(Array(1, 2, 3))`.

Actually you made me realise a good point: this isn’t testing that
we can serialise collection, it’s just testing serialisation of
arrays.  We already test this elsewhere so I’m going to drop
test_array altogether.
Attachment #8966698 - Attachment is obsolete: true
Comment on attachment 8966664 [details]
Bug 1453009 - Test serialisation of collections in wdspec.

https://reviewboard.mozilla.org/r/235364/#review241290

> Actually you made me realise a good point: this isn’t testing that
> we can serialise collection, it’s just testing serialisation of
> arrays.  We already test this elsewhere so I’m going to drop
> test_array altogether.

Actually, scratch that.  I’ve reverted the test_array test since
it is part of element.isCollection in Marionette and we also don’t
appear to test it elsewhere.  I guess testing it here is better
than nowhere.

I’ve also made it return an array of primitives like you suggested.
By a mistake I pushed the review to mozreview when in the middle
of a rebase, which unintelligently means mozreview has lost track
of the r+ to the second patch.  I made no modifications to that
whilst rebasing.
Comment on attachment 8966664 [details]
Bug 1453009 - Test serialisation of collections in wdspec.

https://reviewboard.mozilla.org/r/235364/#review241832

::: testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py:34
(Diff revision 5)
> +        function func() {
> +            return arguments;
> +        }
> +        resolve(func("foo", "bar"));
> +        """)
> +    assert_success(response, [u"foo", u"bar"])

Much better with only that single assert.

::: testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py:46
(Diff revision 5)
> +        """)
> +    assert_success(response, [1, 2])
> +
> +
> +def test_file_list(session, tempfile):
> +    files = [tempfile, tempfile]

Note that you still try to attach the same file twice. Are you sure that browsers allow that?
Attachment #8966664 - Flags: review?(hskupin) → review-
Comment on attachment 8967034 [details]
Bug 1453009 - Remove collection tests from Marionette.

https://reviewboard.mozilla.org/r/235708/#review241834
Attachment #8967034 - Flags: review?(hskupin) → review+
Comment on attachment 8966664 [details]
Bug 1453009 - Test serialisation of collections in wdspec.

https://reviewboard.mozilla.org/r/235364/#review241832

> Note that you still try to attach the same file twice. Are you sure that browsers allow that?

You’re right.  I thought this would create two unique files but
that is not the case.  I’ve updated the patch so two unique files
are created.
Comment on attachment 8966664 [details]
Bug 1453009 - Test serialisation of collections in wdspec.

https://reviewboard.mozilla.org/r/235364/#review241918

::: testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py:43
(Diff revisions 5 - 6)
>  
>      session.url = inline("<input type=file multiple>")
>      upload = session.find.css("input", all=False)
>      for file in files:
> +        file.write("morn morn")
>          upload.send_keys(str(file))

Note, better would be to use `fspath` given that this is Unicode aware, but here we actually don't have such a character in the path.

https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozcrash/tests/conftest.py#6
Attachment #8966664 - Flags: review?(hskupin) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/172094bca9a6
Test serialisation of collections in wdspec. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/d87d4e86f054
Remove collection tests from Marionette. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/172094bca9a6
https://hg.mozilla.org/mozilla-central/rev/d87d4e86f054
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10460 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: