Closed
Bug 1455282
Opened 6 years ago
Closed 6 years ago
Port Marionette close window tests to wdspec
Categories
(Testing :: geckodriver, enhancement, P1)
Testing
geckodriver
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
Attachments
(3 files)
We have a couple of tests for Marionette which test closing a window (tab). We should port those to wdspec.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8969576 -
Flags: review?(ato)
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
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 9•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 10•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•6 years ago
|
||
mozreview-review |
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+
Comment 14•6 years ago
|
||
(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. (-:
Comment 15•6 years ago
|
||
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
Comment 16•6 years ago
|
||
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)
Assignee | ||
Comment 17•6 years ago
|
||
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)
Assignee | ||
Comment 18•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•6 years ago
|
||
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
Comment 21•6 years ago
|
||
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
Comment 23•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e60e61507ef4 https://hg.mozilla.org/mozilla-central/rev/90908c3a6f9e https://hg.mozilla.org/mozilla-central/rev/2ec2ee71cc27
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/w3c/web-platform-tests/pull/10615 * continuous-integration/travis-ci/pr (https://travis-ci.org/w3c/web-platform-tests/builds/370780512?utm_source=github_status&utm_medium=notification)
Updated•6 years ago
|
Upstream PR merged
Assignee | ||
Updated•6 years ago
|
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.
Description
•