Closed Bug 1453009 Opened 7 years ago Closed 7 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: 7 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: