1392984 - Create find elements wdspec tests

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

[David Burns :automatedtester]

We need find element tests for wdspec

Comment 1

[David Burns :automatedtester]

Attachment: Bug 1392984 - Add Find Element wdspec tests

This adds tests for the Find Element command in
https://w3c.github.io/webdriver/webdriver-spec.html#find-element

Review commit: https://reviewboard.mozilla.org/r/172112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172112/

Comment 2

[David Burns :automatedtester]

Attachment: Bug 1392984 - Add Find Elements wdspec tests

This adds tests for the Find Elements command in
https://w3c.github.io/webdriver/webdriver-spec.html#find-element

Review commit: https://reviewboard.mozilla.org/r/172114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172114/

Comment 3

[David Burns :automatedtester]

Attachment: Bug 1392984 - Add Find Element From Element wdspec tests

This adds wdspec tests for Find Element from Element as in
https://w3c.github.io/webdriver/webdriver-spec.html#find-element-from-element

Review commit: https://reviewboard.mozilla.org/r/172116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172116/

Comment 4

[David Burns :automatedtester]

Attachment: Bug 1392984 - Add Find Elements From Element wdspec tests

This adds tests for the Find Elements from Element command in
https://w3c.github.io/webdriver/webdriver-spec.html#find-elements-from-element

Review commit: https://reviewboard.mozilla.org/r/172118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172118/

Comment 5

[David Burns :automatedtester]

Attachment: Bug 1392984 - Update manifest and meta data for find element wdspec tests

Review commit: https://reviewboard.mozilla.org/r/172120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172120/

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900680 [details]
Bug 1392984 - Add Find Element wdspec tests

https://reviewboard.mozilla.org/r/172112/#review177954

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element.py:16
(Diff revision 1)
> +                                  {"using": using, "value": value})
> +
> +
> +# 12.2 Find Element
> +
> [e-mail]("using",[("a"), (True), (None), (1), ([]), ({})])

Space after ",".

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element.py:16
(Diff revision 1)
> +                                  {"using": using, "value": value})
> +
> +
> +# 12.2 Find Element
> +
> [e-mail]("using",[("a"), (True), (None), (1), ([]), ({})])

I don’t think these need to be tuples, do they?  The way this is
written, you are passing find_element(session, ("a"), "value").

I think actually a better way to parametrize this would be:

> @pytest.mark.parametrize("using", [("a", None), (True, None), …])
> def test_invalid_using_argument(session, locator):
>     response = find_element(session, *locator)
>     …

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element.py:23
(Diff revision 1)
> +    # Step 1 - 2
> +    response = find_element(session, using, "value")
> +    assert_error(response, "invalid argument")
> +
> +
> [e-mail]("value", [(None), ([]), ({})])

This has the same problem as the above.

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element.py:36
(Diff revision 1)
> +    result = find_element(session, "css selector", "foo")
> +
> +    assert_error(result, "no such window")

Use "response" consistently.

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element.py:41
(Diff revision 1)
> [e-mail]("using,value",
> +                         [("css selector", "#linkText"),
> +                          ("link text", "full link text"),
> +                          ("partial link text", "link text"),
> +                          ("tag name", "a"),
> +                          ("xpath", "//a")])
> +def test_find_element(session, using, value):
> +    # Step 8 - 9
> +    session.url = inline("<a href=# id=linkText>full link text</a>")
> +
> +    response = find_element(session, using, value)
> +    assert_success(response)

Excellent test and use of parametrization!

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element.py:57
(Diff revision 1)
> +    assert_success(response)
> +
> +
> +def test_no_element(session):
> +    # Step 8 - 9
> +    response  = find_element(session, "css selector", "#wontExist")

We could do the decency of also parametrizing this test?  Low effort, big win I think.

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

https://reviewboard.mozilla.org/r/172114/#review177958

::: testing/web-platform/tests/webdriver/tests/retrieval/find_elements.py:13
(Diff revision 1)
> [e-mail]("using",[("a"), (True), (None), (1), ([]), ({})])
> +def test_invalid_using_argument(session, using):
> +    # Step 1 - 2
> +    response = find_elements(session, using, "value")
> +    assert_error(response, "invalid argument")
> +
> +
> [e-mail]("value", [(None), ([]), ({})])
> +def test_invalid_selector_argument(session, value):
> +    # Step 3 - 4
> +    response = find_elements(session, "css selector", value)
> +    assert_error(response, "invalid argument")

These test exhibit the same problem as the ones in the previous
patch: they appear to be passing tuples of the invalid datatype to
the "using" and "value" fields.

::: testing/web-platform/tests/webdriver/tests/retrieval/find_elements.py:55
(Diff revision 1)
> +def test_no_element(session):
> +    # Step 8 - 9
> +    response = find_elements(session, "css selector", "#wontExist")
> +    assert_success(response)
> +    assert response.body["value"] == []

Could you parametrize this test too?

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900682 [details]
Bug 1392984 - Add Find Element From Element wdspec tests

https://reviewboard.mozilla.org/r/172116/#review177960

::: commit-message-63fe2:1
(Diff revision 1)
> +Bug 1392984 - Add Find element from element wdspec tests r?ato

s/from element/From Element/

::: commit-message-63fe2:3
(Diff revision 1)
> +Bug 1392984 - Add Find element from element wdspec tests r?ato
> +
> +This adds wdspec tests for Find Element from Element as in

s/from/From/

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_element.py:14
(Diff revision 1)
> [e-mail]("using",[("a"), (True), (None), (1), ([]), ({})])
> +def test_invalid_using_argument(session, using):
> +    # Step 1 - 2
> +    response = find_element(session, "notReal", using, "value")
> +    assert_error(response, "invalid argument")
> +
> +
> [e-mail]("value", [(None), ([]), ({})])
> +def test_invalid_selector_argument(session, value):
> +    # Step 3 - 4
> +    response = find_element(session, "notReal", "css selector", value)
> +    assert_error(response, "invalid argument")

Same problems as before.

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_element.py:14
(Diff revision 1)
> +    return session.transport.send("POST",
> +                                  "session/%s/element/%s/element" % (session.session_id, element),
> +                                  {"using": using, "value": value})
> +
> +
> [e-mail]("using",[("a"), (True), (None), (1), ([]), ({})])

Space after ",".

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_element.py:53
(Diff revision 1)
> +def test_no_element(session):
> +    # Step 8 - 9
> +    session.url = inline("<div></div>")
> +    element = session.find.css("div", all=False )
> +    response = find_element(session, element.id, "css selector", "#wontExist")
> +    assert_error(response, "no such element")

Parametrize I think would be a huge win here.

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_element.py:56
(Diff revision 1)
> +
> +
> +def test_no_element(session):
> +    # Step 8 - 9
> +    session.url = inline("<div></div>")
> +    element = session.find.css("div", all=False )

Space before ")".

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900683 [details]
Bug 1392984 - Add Find Elements From Element wdspec tests

https://reviewboard.mozilla.org/r/172118/#review177962

::: commit-message-ccab7:1
(Diff revision 1)
> +Bug 1392984 - Add Find elements from element wdspec tests r?ato

s/elements from element/Elements From Element/

::: commit-message-ccab7:3
(Diff revision 1)
> +Bug 1392984 - Add Find elements from element wdspec tests r?ato
> +
> +This adds tests for the Find Elements from Element command in

s/from/From/

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_elements.py:13
(Diff revision 1)
> +def find_elements(session, element, using, value):
> +    return session.transport.send("POST",
> +                                  "session/%s/element/%s/elements" % (session.session_id, element),
> +                                  {"using": using, "value": value})
> +
> [e-mail]("using",[("a"), (True), (None), (1), ([]), ({})])

Space after ",".

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_elements.py:13
(Diff revision 1)
> [e-mail]("using",[("a"), (True), (None), (1), ([]), ({})])
> +def test_invalid_using_argument(session, using):
> +    # Step 1 - 2
> +    response = find_elements(session, "notReal", using, "value")
> +    assert_error(response, "invalid argument")
> +
> +
> [e-mail]("value", [(None), ([]), ({})])
> +def test_invalid_selector_argument(session, value):
> +    # Step 3 - 4
> +    response = find_elements(session, "notReal", "css selector", value)
> +    assert_error(response, "invalid argument")

Tuples are being passed to "using" and "value".

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_elements.py:33
(Diff revision 1)
> +    result = find_elements(session, "notReal", "css selector", "foo")
> +
> +    assert_error(result, "no such window")

Use "response" consistently.

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_elements.py:52
(Diff revision 1)
> +def test_no_element(session):
> +    # Step 8 - 9
> +    session.url = inline("<div></div>")
> +    element = session.find.css("div", all=False)
> +    response = find_elements(session, element.id, "css selector", "#wontExist")
> +    assert response.body["value"] == []

Parametrize.

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900684 [details]
Bug 1392984 - Update manifest and meta data for find element wdspec tests

https://reviewboard.mozilla.org/r/172120/#review177964

::: commit-message-19806:1
(Diff revision 1)
> +Bug 1392984 - Update Manifest and Meta data for find element wdspec tests r?ato

s/Manifest/manifest/
s/Meta/meta/

Comment 11

[David Burns :automatedtester]

Comment on attachment 8900680 [details]
Bug 1392984 - Add Find Element wdspec tests

https://reviewboard.mozilla.org/r/172112/#review177954

> I don’t think these need to be tuples, do they?  The way this is
> written, you are passing find_element(session, ("a"), "value").
> 
> I think actually a better way to parametrize this would be:
> 
> > @pytest.mark.parametrize("using", [("a", None), (True, None), …])
> > def test_invalid_using_argument(session, locator):
> >     response = find_element(session, *locator)
> >     …

According to [pytest site](https://docs.pytest.org/en/latest/parametrize.html#pytest-mark-parametrize-parametrizing-test-functions) this is correct usage. 

I also think your approach makes it harder to see what is using. The expansion of lists doesn't seem like a good use when pytest can name the items in the tuple. Again I don't see value in that approach when we are showing a specific argument is causing the error.

Comment 12

[David Burns :automatedtester]

Comment on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

https://reviewboard.mozilla.org/r/172114/#review177958

> These test exhibit the same problem as the ones in the previous
> patch: they appear to be passing tuples of the invalid datatype to
> the "using" and "value" fields.

as in the previous patch for find_element.py

Comment 13

[David Burns :automatedtester]

Comment on attachment 8900680 [details]
Bug 1392984 - Add Find Element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172112/diff/1-2/

Comment 14

[David Burns :automatedtester]

Comment on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172114/diff/1-2/

Comment 15

[David Burns :automatedtester]

Comment on attachment 8900682 [details]
Bug 1392984 - Add Find Element From Element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172116/diff/1-2/

Comment 16

[David Burns :automatedtester]

Comment on attachment 8900683 [details]
Bug 1392984 - Add Find Elements From Element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172118/diff/1-2/

Comment 17

[David Burns :automatedtester]

Comment on attachment 8900684 [details]
Bug 1392984 - Update manifest and meta data for find element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172120/diff/1-2/

Comment 18

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900680 [details]
Bug 1392984 - Add Find Element wdspec tests

https://reviewboard.mozilla.org/r/172112/#review177954

> According to [pytest site](https://docs.pytest.org/en/latest/parametrize.html#pytest-mark-parametrize-parametrizing-test-functions) this is correct usage. 
> 
> I also think your approach makes it harder to see what is using. The expansion of lists doesn't seem like a good use when pytest can name the items in the tuple. Again I don't see value in that approach when we are showing a specific argument is causing the error.

That documentation you linked to says the opposite, really.  And
you’re not really using tuples here.  (True) is a grouping and
would need to be (True,) in order to become a tuple.

I’m convinced by your argument to just use the value here, but
I’m reopening this issue so you can remove the parenthesis.

Comment 19

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900680 [details]
Bug 1392984 - Add Find Element wdspec tests

https://reviewboard.mozilla.org/r/172112/#review184300

Comment 20

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

https://reviewboard.mozilla.org/r/172114/#review177958

> as in the previous patch for find_element.py

So I was incorrect earlier to say this was an invalid data type.
(True) is a grouping of True, not a tuple, which would be (True,).
So it will be safe to drop all the parenthesis here.

Comment 21

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

https://reviewboard.mozilla.org/r/172114/#review184308

Comment 22

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900682 [details]
Bug 1392984 - Add Find Element From Element wdspec tests

https://reviewboard.mozilla.org/r/172116/#review184316

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_element.py:14
(Diff revision 2)
> +    return session.transport.send("POST",
> +                                  "session/%s/element/%s/element" % (session.session_id, element),
> +                                  {"using": using, "value": value})
> +
> +
> [e-mail]("using", [("a"), (True), (None), (1), ([]), ({})])

Drop () here.

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_element.py:21
(Diff revision 2)
> +    # Step 1 - 2
> +    response = find_element(session, "notReal", using, "value")
> +    assert_error(response, "invalid argument")
> +
> +
> [e-mail]("value", [(None), ([]), ({})])

Drop parenthesis around None, [], and {}.

Comment 23

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900683 [details]
Bug 1392984 - Add Find Elements From Element wdspec tests

https://reviewboard.mozilla.org/r/172118/#review184318

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_elements.py:13
(Diff revision 2)
> +def find_elements(session, element, using, value):
> +    return session.transport.send("POST",
> +                                  "session/%s/element/%s/elements" % (session.session_id, element),
> +                                  {"using": using, "value": value})
> +
> [e-mail]("using", [("a"), (True), (None), (1), ([]), ({})])

Drop parenthesis around elements in the array.

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_elements.py:20
(Diff revision 2)
> +    # Step 1 - 2
> +    response = find_elements(session, "notReal", using, "value")
> +    assert_error(response, "invalid argument")
> +
> +
> [e-mail]("value", [(None), ([]), ({})])

Drop parenthesis around elements in the array.

Comment 24

[David Burns :automatedtester]

Comment on attachment 8900680 [details]
Bug 1392984 - Add Find Element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172112/diff/2-3/

Comment 25

[David Burns :automatedtester]

Comment on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172114/diff/2-3/

Comment 26

[David Burns :automatedtester]

Comment on attachment 8900682 [details]
Bug 1392984 - Add Find Element From Element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172116/diff/2-3/

Comment 27

[David Burns :automatedtester]

Comment on attachment 8900683 [details]
Bug 1392984 - Add Find Elements From Element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172118/diff/2-3/

Comment 28

[David Burns :automatedtester]

Comment on attachment 8900684 [details]
Bug 1392984 - Update manifest and meta data for find element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172120/diff/2-3/

Comment 29

[David Burns :automatedtester]

Comment on attachment 8900680 [details]
Bug 1392984 - Add Find Element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172112/diff/3-4/

Comment 30

[David Burns :automatedtester]

Comment on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172114/diff/3-4/

Comment 31

[David Burns :automatedtester]

Comment on attachment 8900682 [details]
Bug 1392984 - Add Find Element From Element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172116/diff/3-4/

Comment 32

[David Burns :automatedtester]

Comment on attachment 8900683 [details]
Bug 1392984 - Add Find Elements From Element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172118/diff/3-4/

Comment 33

[David Burns :automatedtester]

Comment on attachment 8900684 [details]
Bug 1392984 - Update manifest and meta data for find element wdspec tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172120/diff/3-4/

Comment 34

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900680 [details]
Bug 1392984 - Add Find Element wdspec tests

https://reviewboard.mozilla.org/r/172112/#review184400

Comment 35

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

https://reviewboard.mozilla.org/r/172114/#review184404

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4f917de7d59
Add Find Element wdspec tests r=ato
https://hg.mozilla.org/integration/autoland/rev/f77826e523e5
Add Find Elements wdspec tests r=ato
https://hg.mozilla.org/integration/autoland/rev/3ebe0b1a5fa2
Add Find Element From Element wdspec tests r=ato
https://hg.mozilla.org/integration/autoland/rev/9b7b66644b99
Add Find Elements From Element wdspec tests r=ato
https://hg.mozilla.org/integration/autoland/rev/ecb5efcf0fee
Update manifest and meta data for find element wdspec tests r=ato

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b4f917de7d59
https://hg.mozilla.org/mozilla-central/rev/f77826e523e5
https://hg.mozilla.org/mozilla-central/rev/3ebe0b1a5fa2
https://hg.mozilla.org/mozilla-central/rev/9b7b66644b99
https://hg.mozilla.org/mozilla-central/rev/ecb5efcf0fee