David Burns :automatedtester Assignee
	Description • 7 years ago

We should start creating new WPT tests for WebDriver and Window Rect is a good one to start with.

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8867684 [details]
Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

https://reviewboard.mozilla.org/r/139270/#review158000

I want to ask you to split the tests for Get Window Rect and Set Window Rect in two separate files, as we discussed on IRC yesterday.

I also want to see tests for floats, both for input and return types.  I think the specification says the return type should be a JSON Number, meaning it can either be an integer or an float.

Regarding moving the window to negative coordinates, I’ve outlined what works on different platforms in https://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py#62-104.  Even if we don’t run wdspec tests on these platforms, I think it would be good to encode this knowledge.

::: testing/web-platform/tests/webdriver/window_rect.py:20
(Diff revision 4)
> +def test_get_window_rect_alert_prompt(session):
> +    # Step 2
> +    session.url = alert_doc
> +
> +    result = session.transport.send("GET", "session/%s/window/rect" % session.session_id)
> +
> +    assert_error(result, "unexpected alert open")

This is not really an exhaustive test of the user prompt handler.  See https://github.com/w3c/web-platform-tests/blob/master/webdriver/get_title.py for some examples.

Unfortunately that doesn’t seem to have made it into mozilla-central yet, so I’m not sure all the fixtures needed for that are available in-tree.  For this reason, I’m happy to accept this patch as-is if you file a followup bug to address user prompt handling for Set Window Rect and Get Window Rect commands.

::: testing/web-platform/tests/webdriver/window_rect.py:33
(Diff revision 4)
> +    assert result.status == 200
> +    assert isinstance(result.body["value"], dict)

I think the assert_success fixture handles most of this.

::: testing/web-platform/tests/webdriver/window_rect.py:92
(Diff revision 4)
> +    session.execute_script("window.document.documentElement.requestFullscreen()")
> +    result = session.transport.send("POST",
> +                                    "session/%s/window/rect" % session.session_id,
> +                                    {"width": 400, "height": 400})

The first line here isn’t synchronous, so there’s no guarantee that :93 will not resize the window before it has been fullscreened.

I would call session.fullscreen() instead, which is supposed to give you a blocking guarantee.  Since you call session.minimize() below, this gives nice parity.

::: testing/web-platform/tests/webdriver/window_rect.py:97
(Diff revision 4)
> +    assert_success(result, {"x": original["x"],
> +                            "y": original["y"],
> +                            "width": 400, "height": 400})

width and height should also be split on separate lines.

::: testing/web-platform/tests/webdriver/window_rect.py:104
(Diff revision 4)
> +                            "width": 400, "height": 400})
> +
> +# TODO: We don't currently support minimize
> +def test_set_window_rect_window_minimized(session):
> +    # step 11
> +    session.minimize("window.minimize()")

I don’t think this command takes an argument.

::: testing/web-platform/tests/webdriver/window_rect.py:105
(Diff revision 4)
> +
> +# TODO: We don't currently support minimize
> +def test_set_window_rect_window_minimized(session):
> +    # step 11
> +    session.minimize("window.minimize()")
> +    assert session.execute_script("return document.getAttribute('hidden')")

Presumably you want the document.hidden property, and not the IDL attribute.

::: testing/web-platform/tests/webdriver/window_rect.py:109
(Diff revision 4)
> +    session.minimize("window.minimize()")
> +    assert session.execute_script("return document.getAttribute('hidden')")
> +    result = session.transport.send("POST",
> +                                    "session/%s/window/rect" % session.session_id,
> +                                    {"width": 400, "height": 400})
> +    assert not session.execute_script("return document.getAttribute('hidden')")

Same here.

::: testing/web-platform/tests/webdriver/window_rect.py:139
(Diff revision 4)
> +    _max = session.execute_script("""
> +        return {
> +          width: window.screen.availWidth,
> +          height: window.screen.availHeight,
> +        }""")

Drop leading underscore.

::: testing/web-platform/tests/webdriver/window_rect.py:149
(Diff revision 4)
> +    # Step 14
> +    assert_success(result, {"x": original["x"], "y": original["y"],
> +                            "width": _max["width"],
> +                            "height": _max["height"]})

I’m not sure this test will work equally well on all platforms.

::: testing/web-platform/tests/webdriver/window_rect.py:158
(Diff revision 4)
> +
> +
> +def test_set_window_height_width_as_current(session):
> +    # Step 12
> +    get_response = session.transport.send("GET", "session/%s/window/rect" % session.session_id)
> +    original = get_response.body["value"]

You probably want to run assert_success on all these and get the return value from that.

::: testing/web-platform/tests/webdriver/window_rect.py:161
(Diff revision 4)
> +    # Step 12
> +    get_response = session.transport.send("GET", "session/%s/window/rect" % session.session_id)
> +    original = get_response.body["value"]
> +    result = session.transport.send("POST",
> +                                    "session/%s/window/rect" % session.session_id,
> +                                    {"width": int(original["width"]),

The integer conversion should not be necessary.

::: testing/web-platform/tests/webdriver/window_rect.py:177
(Diff revision 4)
> +    # Step 13
> +    get_response = session.transport.send("GET", "session/%s/window/rect" % session.session_id)
> +    original = get_response.body["value"]
> +    result = session.transport.send("POST",
> +                                    "session/%s/window/rect" % session.session_id,
> +                                    {"x": int(original["x"]) + 10,

Incorrect integer conversions.

::: testing/web-platform/tests/webdriver/window_rect.py:190
(Diff revision 4)
> +    # Step 13
> +    result = session.transport.send("POST",
> +                                    "session/%s/window/rect" % session.session_id,
> +                                    {"x": - 8,
> +                                     "y": - 8})
> +    # Step 14
> +    assert_success(result, {"x": -8, "y": 23,
> +                            "width": original["width"],
> +                            "height": original["height"]})

Again I’m not sure this will work cross platform.

	David Burns :automatedtester Assignee
	Comment 7 • 7 years ago
mozreview-review-reply

Comment on attachment 8867684 [details]
Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

https://reviewboard.mozilla.org/r/139270/#review158000

> This is not really an exhaustive test of the user prompt handler.  See https://github.com/w3c/web-platform-tests/blob/master/webdriver/get_title.py for some examples.
> 
> Unfortunately that doesn’t seem to have made it into mozilla-central yet, so I’m not sure all the fixtures needed for that are available in-tree.  For this reason, I’m happy to accept this patch as-is if you file a followup bug to address user prompt handling for Set Window Rect and Get Window Rect commands.

I am happy to drop this test and we can add the step 2 tests when we have finished the current wpt sync

> Drop leading underscore.

max is a stdlib function so if I have a variable with its name it won't work

	David Burns :automatedtester Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8867684 [details]
Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

https://reviewboard.mozilla.org/r/139270/#review158000

> The integer conversion should not be necessary.

It is required. Get Window Rect returns a float and Set Window Rect requires integers.

> Incorrect integer conversions.

It is required. Get Window Rect returns a float and Set Window Rect requires integers.

	David Burns :automatedtester Assignee
	Comment 9 • 7 years ago
mozreview-review-reply

Comment on attachment 8867684 [details]
Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

https://reviewboard.mozilla.org/r/139270/#review158000

> I think the assert_success fixture handles most of this.

`assert_success` doesnt really fit in this because I dont want to test what are the values for the keys in the result but their structure.

	David Burns :automatedtester Assignee
	Comment 10 • 7 years ago
mozreview-review-reply

Comment on attachment 8867684 [details]
Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

https://reviewboard.mozilla.org/r/139270/#review158000

> You probably want to run assert_success on all these and get the return value from that.

`assert_success` doesn't return anything

	Andreas Tolfsen ❲:ato❳
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8891080 [details]
Bug 1364389 - Update capabilities with value returned from the remote end

https://reviewboard.mozilla.org/r/162260/#review167742

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8867684 [details]
Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

https://reviewboard.mozilla.org/r/139270/#review167746

This is much better, thanks!

Can you trigger a try run that includes the Wd job?  It only runs on Linux x86 opt and debug currently.

::: testing/web-platform/tests/webdriver/tests/get_window_rect.py:82
(Diff revision 6)
> +    assert isinstance(result.body["value"]["width"], float)
> +    assert isinstance(result.body["value"]["height"], float)
> +    assert isinstance(result.body["value"]["x"], float)
> +    assert isinstance(result.body["value"]["y"], float)

I think these can technically be either float or int, because the JSON type here is a JSON Number.

The correct check would be:

> assert isinstance(result.body["value"]["width"], (int, float))

::: testing/web-platform/tests/webdriver/tests/set_window_rect.py:109
(Diff revision 6)
> +def test_set_window_fullscreen(session):
> +    get_response = session.transport.send("GET", "session/%s/window/rect" % session.session_id)
> +    original = get_response.body["value"]
> +
> +    # step 10
> +    session.execute_script("window.document.documentElement.requestFullscreen()")

There is a fullscreen command to achieve this which is also synchronous.

::: testing/web-platform/tests/webdriver/tests/set_window_rect.py:109
(Diff revision 6)
> +def test_set_window_fullscreen(session):
> +    get_response = session.transport.send("GET", "session/%s/window/rect" % session.session_id)
> +    original = get_response.body["value"]
> +
> +    # step 10
> +    session.execute_script("window.document.documentElement.requestFullscreen()")

You probably also want an assertion that we are actually in fullscreen after this line, like you have below for document.hidden.

::: testing/web-platform/tests/webdriver/tests/set_window_rect.py:168
(Diff revision 6)
> +    assert_success(result, {"x": original["x"], "y": original["y"],
> +                            "width": _max["width"],
> +                            "height": _max["height"]})

It is possible in some WMs to make the window larger than the screen.  The correct assertion here is to check that it is greater than or equal to the availHeight/availWidth.

::: testing/web-platform/tests/webdriver/tests/set_window_rect.py:179
(Diff revision 6)
> +                                    {"width": int(original["width"]),
> +                                     "height": int(original["height"])})

FWIW I filed https://github.com/w3c/webdriver/issues/988 about parity of the type checks of width/height between {Get,Set} Window Rect.

::: testing/web-platform/tests/webdriver/tests/set_window_rect.py:183
(Diff revision 6)
> +    assert_success(result, {"x": original["x"],
> +                            "y": original["y"],
> +                            "width": original["width"],
> +                            "height": original["height"]})

You also want a test that only sets width and only sets height.  This is a known problem in Marionette.

	Andreas Tolfsen ❲:ato❳
	Comment 15 • 7 years ago
mozreview-review-reply

Comment on attachment 8867684 [details]
Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

https://reviewboard.mozilla.org/r/139270/#review167746

> You also want a test that only sets width and only sets height.  This is a known problem in Marionette.

Actually, never mind with this.  I will address this as part of https://github.com/mozilla/geckodriver/issues/643.

	Andreas Tolfsen ❲:ato❳
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8867684 [details]
Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

https://reviewboard.mozilla.org/r/139270/#review168070

::: testing/web-platform/tests/webdriver/tests/set_window_rect.py:111
(Diff revisions 6 - 7)
>      original = get_response.body["value"]
>  
>      # step 10
> -    session.execute_script("window.document.documentElement.requestFullscreen()")
> +    session.transport.send("POST",
> +                           "session/%s/window/fullscreen" % session.session_id)
> +    assert not session.execute_script("return document.fullscreenEnabled")

s/not//

::: testing/web-platform/tests/webdriver/tests/set_window_rect.py:111
(Diff revisions 6 - 7)
>      original = get_response.body["value"]
>  
>      # step 10
> -    session.execute_script("window.document.documentElement.requestFullscreen()")
> +    session.transport.send("POST",
> +                           "session/%s/window/fullscreen" % session.session_id)
> +    assert not session.execute_script("return document.fullscreenEnabled")

fullscreenEnabled tells you if full screen mode is available, not if you activated it.  You want document.fullscreen here.

	Andreas Tolfsen ❲:ato❳
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8867684 [details]
Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

https://reviewboard.mozilla.org/r/139270/#review168764

	Andreas Tolfsen ❲:ato❳
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8892297 [details]
Bug 1364389 - Remove superfluous test

https://reviewboard.mozilla.org/r/163240/#review168766

	Mozilla Autoland
	Comment 24 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a5898f35a409 -d c291fa5c4c8a: rebasing 410885:a5898f35a409 "Bug 1364389 - Create Window Rect Webdriver Web Platform Tests. r=ato"
merging testing/web-platform/meta/MANIFEST.json
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 28 • 7 years ago

Pushed by dburns@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7364b462fa2e
Create Window Rect Webdriver Web Platform Tests. r=ato
https://hg.mozilla.org/integration/autoland/rev/468dc861fec0
Update capabilities with value returned from the remote end r=ato
https://hg.mozilla.org/integration/autoland/rev/974265b68434
Remove superfluous test r=ato

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 29 • 7 years ago

Backed out for failing linting: webdriver/tests/get_window_rect.py in source but not in manifest:

https://hg.mozilla.org/integration/autoland/rev/03e835b964d974fd6948baf77396e49068a4ffbb
https://hg.mozilla.org/integration/autoland/rev/040d3a9a4d6338f4ef4f793aed66bee559c51875
https://hg.mozilla.org/integration/autoland/rev/bede9639030cd8a7b19df1653f8c6479a0b3609d

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=974265b68434c13ac5ae1e1799629738a2c13563&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120090024&repo=autoland
> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | webdriver/tests/get_window_rect.py in source but not in manifest. (wpt-manifest)

	Pulsebot
	Comment 33 • 7 years ago

Pushed by dburns@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4deecfb7f7f3
Create Window Rect Webdriver Web Platform Tests. r=ato
https://hg.mozilla.org/integration/autoland/rev/d04626de94f3
Update capabilities with value returned from the remote end r=ato
https://hg.mozilla.org/integration/autoland/rev/252680d7f5b2
Remove superfluous test r=ato

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 34 • 7 years ago

Backed out for linting failure: webdriver/tests/.cache/v/cache/lastfailed in manifest but removed from source:

https://hg.mozilla.org/integration/autoland/rev/a0424962148f6285351f5483a46f6859c09a31f4
https://hg.mozilla.org/integration/autoland/rev/7c627ec00094e467b784a26a1be3440abbcdd76d
https://hg.mozilla.org/integration/autoland/rev/a688c94e7fddd111a29388484fd3650715219339

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=252680d7f5b2f88bffc2a42f3f700ad134a9738a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120119435&repo=autoland
> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | webdriver/tests/.cache/v/cache/lastfailed in manifest but removed from source. (wpt-manifest)

	Pulsebot
	Comment 38 • 7 years ago

Pushed by dburns@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b19bd2e4238c
Create Window Rect Webdriver Web Platform Tests. r=ato
https://hg.mozilla.org/integration/autoland/rev/dcb5dc55efd1
Update capabilities with value returned from the remote end r=ato
https://hg.mozilla.org/integration/autoland/rev/d2860f1e69a6
Remove superfluous test r=ato

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 39 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b19bd2e4238c
https://hg.mozilla.org/mozilla-central/rev/dcb5dc55efd1
https://hg.mozilla.org/mozilla-central/rev/d2860f1e69a6