Closed Bug 1455282 Opened 6 years ago Closed 6 years ago

Port Marionette close window tests to wdspec

Categories

(Testing :: geckodriver, enhancement, P1)

Product:
Testing
Component:
geckodriver
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(3 files)

Bug 1455282 - [wdclient] End session if no more windows are open.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1455282 - [wdspec] Add tests for Close Window command.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1455282 - [wdspec] Ignore exceptions when switching to top-level browsing context.
59 bytes, text/x-review-board-request
ato
: review+
Details 
We have a couple of tests for Marionette which test closing a window (tab). We should port those to wdspec.
Please note that we cannot delete the existent Marionette tests yet, because it would mean that we would loose coverage for Windows, MacOS, and Android.
Attachment #8969576 - Flags: review?(ato)
Comment on attachment 8969577 [details]
Bug 1455282 - [wdspec] Add tests for Close Window command.

https://reviewboard.mozilla.org/r/238336/#review244140

::: testing/web-platform/meta/webdriver/tests/close_window/prompts.py.ini:1
(Diff revision 2)
> +[prompts.py]

I should rename this file to `user_prompts.py` to be in sync with other commands.
Comment on attachment 8969576 [details]
Bug 1455282 - [wdclient] End session if no more windows are open.

https://reviewboard.mozilla.org/r/238334/#review244440
Attachment #8969576 - Flags: review?(ato) → review+
Comment on attachment 8969577 [details]
Bug 1455282 - [wdspec] Add tests for Close Window command.

https://reviewboard.mozilla.org/r/238336/#review244442

::: testing/web-platform/meta/webdriver/tests/close_window/user_prompts.py.ini:1
(Diff revision 3)
> +[user_prompts.py]

Doesn’t match name of the file.

::: testing/web-platform/tests/webdriver/tests/close_window/close.py:26
(Diff revision 3)
> +    value = assert_success(response)
> +    assert new_handle not in value
> +    assert session.handles == handles

Nothing wrong with the way you have written this, but since the
command returns the window handles, you could shorten this:

> assert_success(response, original_handles)

::: testing/web-platform/tests/webdriver/tests/close_window/close.py:35
(Diff revision 3)
> +    value = assert_success(response)
> +    assert value == []

assert_success(response, [])

::: testing/web-platform/tests/webdriver/tests/close_window/close.py:38
(Diff revision 3)
> +    # With no more open top-level browsing contexts, the session is closed.
> +    session.session_id = None

Does the client not do this for you?

::: testing/web-platform/tests/webdriver/tests/close_window/user_prompts.py:23
(Diff revision 3)
> +    """
> +    3. Handle any user prompts and return its value if it is an error.
> +
> +    [...]
> +
> +    In order to handle any user prompts a remote end must take the
> +    following steps:
> +
> +      [...]
> +
> +      2. Perform the following substeps based on the current session's
> +      user prompt handler:
> +
> +        [...]
> +
> +        - accept state
> +           Accept the current user prompt.
> +
> +    """

Can you dispose of these quotes when you make this change, please?

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:79
(Diff revision 3)
>              session.close()
>  
>      session.window_handle = current_window
>  
>  
> +@ignore_exceptions

This may warrant a separate commit.
Attachment #8969577 - Flags: review?(ato) → review+
Comment on attachment 8969577 [details]
Bug 1455282 - [wdspec] Add tests for Close Window command.

https://reviewboard.mozilla.org/r/238336/#review244442

> Doesn’t match name of the file.

Maybe you looked at a former version of the patch? It has already been fixed.

> Nothing wrong with the way you have written this, but since the
> command returns the window handles, you could shorten this:
> 
> > assert_success(response, original_handles)

Oh, wow. That's nice! Thanks for pointing this out. I will add this but leave the other two asserts which are necessary to ensure that we really have the correct state, and `close` didn't just return the expected state without actually closing the window.

> Does the client not do this for you?

The operation here is not `session.close()` but our custom POST request. So the wdclient doesn't detect that, and we have to ensure to reset the session id instead.

> Can you dispose of these quotes when you make this change, please?

Sure, those are annoying and migth not reflect reality anymore.
Comment on attachment 8970362 [details]
Bug 1455282 - [wdspec] Ignore exceptions when switching to top-level browsing context.

https://reviewboard.mozilla.org/r/239148/#review244890

I thought I had reviewed this before…
Attachment #8970362 - Flags: review?(ato) → review+
(In reply to Andreas Tolfsen ‹:ato› from comment #13)

> I thought I had reviewed this before…

Ah, it was moved to a separate commit as requested.  So I’m not
going entirely insane. (-:
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/703161d003be
[wdclient] End session if no more windows are open. r=ato
https://hg.mozilla.org/integration/autoland/rev/843cb0cb835e
[wdspec] Ignore exceptions when switching to top-level browsing context. r=ato
https://hg.mozilla.org/integration/autoland/rev/295fd2df534f
[wdspec] Add tests for Close Window command. r=ato
Backed out 3 changesets (bug 1455282) for linting failure in /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 on a CLOSED TREE

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=295fd2df534f6a5f59f9d620be955a3e70ade74b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=175269249
Log: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c4027f932a5276de4d704103e0d0f0361b94d61a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c4027f932a5276de4d704103e0d0f0361b94d61a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified.
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=175269249&repo=autoland&lineNumber=268

[task 2018-04-24T06:40:22.159Z] No specific files specified, running the full wpt lint (this is slow)
[task 2018-04-24T06:40:34.785Z]  0:12.63 INFO Diffing old and new manifests /builds/worker/checkouts/gecko/testing/web-platform/mozilla/meta/MANIFEST.json
[task 2018-04-24T06:40:34.792Z]  0:12.63 INFO Diffing old and new manifests /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json
[task 2018-04-24T06:40:36.936Z]  0:14.78 /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json  0  error  webdriver/tests/fullscreen_window/user_prompts.py in manifest but removed from source.  (wpt-manifest)
[task 2018-04-24T06:40:36.936Z]  0:14.78 /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json  0  error  webdriver/tests/get_active_element/user_prompts.py in manifest but removed from source.  (wpt-manifest)
[task 2018-04-24T06:40:37.110Z]  0:14.95 ERROR Manifest /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json is outdated, use |mach wpt-manifest-update| to fix.
[task 2018-04-24T06:44:37.173Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | webdriver/tests/fullscreen_window/user_prompts.py in manifest but removed from source. (wpt-manifest)
[task 2018-04-24T06:44:37.174Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | webdriver/tests/get_active_element/user_prompts.py in manifest but removed from source. (wpt-manifest)
[taskcluster 2018-04-24 06:44:37.481Z] === Task Finished ===
[taskcluster 2018-04-24 06:44:37.481Z] Unsuccessful task run with exit code: 1 completed in 515.772 seconds
Flags: needinfo?(hskupin)
Sorry the last manifest update added accidentally two files which are targeted for a different bug locally. I will push a fix, and also check how often user_prompts.py fails.
Flags: needinfo?(hskupin)
The failure is actually again a shutdown hang of Firefox:

[task 2018-04-24T07:00:31.762Z] 07:00:31     INFO - PID 1194 | 1524553231753	Marionette	TRACE	0 -> [0,12,"Marionette:Quit",{"flags":["eForceQuit"]}]
[task 2018-04-24T07:00:31.763Z] 07:00:31     INFO - PID 1194 | 1524553231754	Marionette	TRACE	0 <- [1,12,{"error":"invalid session id","message":"","stacktrace":"WebDriverError@chrome://marionette/content/error.js:178:5\nInv ... et@chrome://marionette/content/server.js:245:8\n_onJSONObjectReady/<@chrome://marionette/content/transport.js:500:9\n"},null]
[task 2018-04-24T07:00:31.764Z] 07:00:31     INFO - PID 1194 | 1524553231754	geckodriver::marionette	TRACE	<- [1,12,{"error":"invalid session id","message":"","stacktrace":"WebDriverError@chrome://marionette/content/error.js:178:5\nInvalidSessionIDError@chrome://marionette/content/error.js:347:5\nassert.that/<@chrome://marionette/content/assert.js:405:13\nassert.session@chrome://marionette/content/assert.js:79:3\ndespatch@chrome://marionette/content/server.js:294:7\nasync*execute@chrome://marionette/content/server.js:271:11\nasync*onPacket/<@chrome://marionette/content/server.js:246:15\nasync*onPacket@chrome://marionette/content/server.js:245:8\n_onJSONObjectReady/<@chrome://marionette/content/transport.js:500:9\n"},null]
[task 2018-04-24T07:00:31.782Z] 07:00:31     INFO - PID 1194 | 1524553231771	Marionette	DEBUG	Closed connection 0
[task 2018-04-24T07:00:57.853Z] 07:00:57     INFO - TEST-UNEXPECTED-TIMEOUT | /webdriver/tests/close_window/user_prompts.py | expected OK

So nothing we can do here right now. If it fails only intermittently we can land.
Blocks: 1456400
The latest try looks fine and shows the shutdown hang not that often. So I'm going to reland the patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e512bc224013bf7744b3661e61b12d2471cdb9a7
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e60e61507ef4
[wdclient] End session if no more windows are open. r=ato
https://hg.mozilla.org/integration/autoland/rev/90908c3a6f9e
[wdspec] Ignore exceptions when switching to top-level browsing context. r=ato
https://hg.mozilla.org/integration/autoland/rev/2ec2ee71cc27
[wdspec] Add tests for Close Window command. r=ato
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10615 for changes under testing/web-platform/tests
No longer blocks: 1456400
Depends on: 1456400
Summary: Port Mariontte close window tests to wdspec → Port Marionette close window tests to wdspec
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: