1517796 - MarionetteTestharnessProtocolPart._close_windows() doesn't close any user prompts

Status: RESOLVED FIXED

(Testing :: web-platform-tests, enhancement, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

While trying to fix a wpt failure as introduced by my patches on bug 1504756 I noticed that the method `MarionetteTestharnessProtocolPart.dismiss_alert()` is a no-op when called from `_close_windows()` because `switch_to_window()` doesn't raise a `UnexpectedAlertOpen` error.

Why does `dismiss_alert()` need a callback at all? Why can't we just use `self.marionette.switch_to_alert()` to check for an existing user prompt, and use `NoAlertPresentException` to return from the function?

https://searchfox.org/mozilla-central/rev/fef7f858efb695a76010b4c624da5277c16e95b3/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py#160

https://searchfox.org/mozilla-central/rev/fef7f858efb695a76010b4c624da5277c16e95b3/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py#177-188

Comment 1

[James Graham [:jgraham]]

The callback is an optimistaion; it means that in the most commn case where there are 0 alerts open we just perform the action directly rather than going through an extra command and an exception, bot of which are slow. It may be that it doesn't matter in this case, but generally putting extra netwerk traffic and exceptions on the common path seems like a bad idea.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Well, in this case we also issue two commands which is the `Switch to Window` one. See line 161 of the first link above. 

But as said above this won't trigger the error which is used to close the left-open alerts, and as such they will remain and cause a real failure later when closing the window.

Comment 3

[James Graham [:jgraham]]

Right, sure there's a bug here that we run `switch_to_window` twice. I'm a little surprised that we don't throw an error, but I guess it makes sense. I think in this case it could make sense to rearrange the commands to:

                self.marionette.switch_to_window(handle)
                self.logger.info("Closing window %s" % handle)
                self.dismiss_alert(lambda: self.marionette.close())

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Not really, given that in case of a user prompt being open, it will close it but not the window.

Comment 5

[James Graham [:jgraham]]

The point of `dismiss_alert` is that it runs the original command until there isn't an unexpected alert open exception and closed the alert when there is. So it would close the window afaict.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh, wait. You are right. I missed that the break statement is in the else case of the try block. Let me just patch that.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2b8a02b6e6c9f33f1aaec4b7643170b6b5f682b

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1517796 - [wpt] Correctly dismiss user prompts in MarionetteTestharnessProtocolPart._close_windows(). r?jgraham

Using the "Switch To Window" command to check if a user prompt is open
doesn't work because that command doesn't raise a "unexpected alert open"
error. To fix that the "Close Window" command can be used for.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcb026c5194b
[wpt] Correctly dismiss user prompts in MarionetteTestharnessProtocolPart._close_windows(). r=jgraham

Comment 10

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14737 for changes under testing/web-platform/tests

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/fcb026c5194b