Closed Bug 1392984 Opened 7 years ago Closed 7 years ago

Create find elements wdspec tests

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Version 3
enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: automatedtester, Assigned: automatedtester)

References

Details

Attachments

(5 files)

We need find element tests for wdspec
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 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 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 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 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+
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 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 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 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 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 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 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 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+
Priority: -- → P1
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 on attachment 8900681 [details]
Bug 1392984 - Add Find Elements wdspec tests

https://reviewboard.mozilla.org/r/172114/#review184404
Attachment #8900681 - Flags: review?(ato) → review+
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
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.