1364389 - Create Window Rect WPT tests

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

[David Burns :automatedtester]

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

Comment 1

[David Burns :automatedtester]

Attachment: Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

These tests exercise the {Get|Set} Window Rect commands.
https://w3c.github.io/webdriver/webdriver-spec.html#get-window-rect
https://w3c.github.io/webdriver/webdriver-spec.html#set-window-rect

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

Comment 2

[David Burns :automatedtester]

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

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

Comment 3

[David Burns :automatedtester]

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

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

Comment 4

[David Burns :automatedtester]

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

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

Comment 5

[David Burns :automatedtester]

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

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

Comment 6

[Andreas Tolfsen ❲:ato❳]

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.

Comment 7

[David Burns :automatedtester]

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

Comment 8

[David Burns :automatedtester]

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.

Comment 9

[David Burns :automatedtester]

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.

Comment 10

[David Burns :automatedtester]

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

Comment 11

[David Burns :automatedtester]

Attachment: Bug 1364389 - Update capabilities with value returned from the remote end

When a new session is created the server returns a few details of the
remote end. It replaces the contents of the capabilities variable with
what is returned so that it can be used when accessing it from the
session object.

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

Comment 12

[David Burns :automatedtester]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139270/diff/5-6/

Comment 13

[Andreas Tolfsen ❲:ato❳]

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

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

Comment 14

[Andreas Tolfsen ❲:ato❳]

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.

Comment 15

[Andreas Tolfsen ❲:ato❳]

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.

Comment 16

[David Burns :automatedtester]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139270/diff/6-7/

Comment 17

[David Burns :automatedtester]

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

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

Comment 18

[Andreas Tolfsen ❲:ato❳]

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.

Comment 19

[David Burns :automatedtester]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139270/diff/7-8/

Comment 20

[David Burns :automatedtester]

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

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

Comment 21

[David Burns :automatedtester]

Attachment: Bug 1364389 - Remove superfluous test

This is tested more extensively with the new * window rect tests.

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

Comment 22

[Andreas Tolfsen ❲:ato❳]

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

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

Comment 23

[Andreas Tolfsen ❲:ato❳]

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

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

Comment 24

[Mozilla Autoland]

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)

Comment 25

[David Burns :automatedtester]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139270/diff/8-9/

Comment 26

[David Burns :automatedtester]

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

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

Comment 27

[David Burns :automatedtester]

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

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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)

Comment 30

[David Burns :automatedtester]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139270/diff/9-10/

Comment 31

[David Burns :automatedtester]

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

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

Comment 32

[David Burns :automatedtester]

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

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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)

Comment 35

[David Burns :automatedtester]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139270/diff/10-11/

Comment 36

[David Burns :automatedtester]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162260/diff/5-6/

Comment 37

[David Burns :automatedtester]

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

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

Comment 38

[Pulsebot]

Pushed by [e-mail]:
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

Comment 39

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

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