bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Create find elements wdspec tests

RESOLVED FIXED in Firefox 57

Status

Testing
Marionette
P1
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: automatedtester, Assigned: automatedtester)

Tracking

Version 3
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

11 months ago
We need find element tests for wdspec
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

11 months ago
mozreview-review
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
> +
> +@pytest.mark.parametrize("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
> +
> +@pytest.mark.parametrize("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")
> +
> +
> +@pytest.mark.parametrize("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)
> +@pytest.mark.parametrize("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.
Attachment #8900680 - Flags: review?(ato) → review-

Comment 7

11 months ago
mozreview-review
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)
> +@pytest.mark.parametrize("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")
> +
> +
> +@pytest.mark.parametrize("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?
Attachment #8900681 - Flags: review?(ato) → review-

Comment 8

11 months ago
mozreview-review
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)
> +@pytest.mark.parametrize("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")
> +
> +
> +@pytest.mark.parametrize("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})
> +
> +
> +@pytest.mark.parametrize("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 ")".
Attachment #8900682 - Flags: review?(ato) → review-

Comment 9

11 months ago
mozreview-review
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})
> +
> +@pytest.mark.parametrize("using",[("a"), (True), (None), (1), ([]), ({})])

Space after ",".

::: testing/web-platform/tests/webdriver/tests/retrieval/find_element_from_elements.py:13
(Diff revision 1)
> +@pytest.mark.parametrize("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")
> +
> +
> +@pytest.mark.parametrize("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.
Attachment #8900683 - Flags: review?(ato) → review-

Comment 10

11 months ago
mozreview-review
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/
Attachment #8900684 - Flags: review?(ato) → review+
(Assignee)

Comment 11

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 12

10 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

10 months ago
mozreview-review-reply
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

10 months ago
mozreview-review
Comment on attachment 8900680 [details]
Bug 1392984 - Add Find Element wdspec tests

https://reviewboard.mozilla.org/r/172112/#review184300
Attachment #8900680 - Flags: review?(ato) → review-

Comment 20

10 months ago
mozreview-review-reply
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

10 months ago
mozreview-review
Comment on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

https://reviewboard.mozilla.org/r/172114/#review184308
Attachment #8900681 - Flags: review?(ato) → review-

Comment 22

10 months ago
mozreview-review
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})
> +
> +
> +@pytest.mark.parametrize("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")
> +
> +
> +@pytest.mark.parametrize("value", [(None), ([]), ({})])

Drop parenthesis around None, [], and {}.
Attachment #8900682 - Flags: review?(ato) → review+

Comment 23

10 months ago
mozreview-review
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})
> +
> +@pytest.mark.parametrize("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")
> +
> +
> +@pytest.mark.parametrize("value", [(None), ([]), ({})])

Drop parenthesis around elements in the array.
Attachment #8900683 - Flags: review?(ato) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Priority: -- → P1

Comment 34

10 months ago
mozreview-review
Comment on attachment 8900680 [details]
Bug 1392984 - Add Find Element wdspec tests

https://reviewboard.mozilla.org/r/172112/#review184400
Attachment #8900680 - Flags: review?(ato) → review+

Comment 35

10 months ago
mozreview-review
Comment on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

https://reviewboard.mozilla.org/r/172114/#review184404
Attachment #8900681 - Flags: review?(ato) → review+

Comment 36

10 months ago
Pushed by dburns@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.