Closed Bug 1364389 Opened 4 years ago Closed 4 years ago

Create Window Rect WPT tests

Categories

(Testing :: Marionette, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: automatedtester, Assigned: automatedtester)

References

Details

(Keywords: pi-marionette-spec)

Attachments

(3 files)

We should start creating new WPT tests for WebDriver and Window Rect is a good one to start with.
Depends on: 1364594
Depends on: 1368264
Depends on: 1370936
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.
Attachment #8867684 - Flags: review?(ato) → review-
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 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 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 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
Depends on: 1378227
Comment on attachment 8891080 [details]
Bug 1364389 - Update capabilities with value returned from the remote end

https://reviewboard.mozilla.org/r/162260/#review167742
Attachment #8891080 - Flags: review?(ato) → 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.
Attachment #8867684 - Flags: review?(ato) → review-
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 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.
Attachment #8867684 - Flags: review?(ato) → review-
Comment on attachment 8867684 [details]
Bug 1364389 - Create Window Rect Webdriver Web Platform Tests.

https://reviewboard.mozilla.org/r/139270/#review168764
Attachment #8867684 - Flags: review?(ato) → review+
Comment on attachment 8892297 [details]
Bug 1364389 - Remove superfluous test

https://reviewboard.mozilla.org/r/163240/#review168766
Attachment #8892297 - Flags: review?(ato) → review+
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)
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
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
Flags: needinfo?(dburns)
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
Flags: needinfo?(dburns)
Depends on: 1376625
No longer depends on: 1378227
You need to log in before you can comment on or make changes to this bug.