Closed Bug 1517796 Opened 2 years ago Closed 2 years ago
Testharness Protocol Part ._close _windows() doesn't close any user prompts
Bug 1517796 - [wpt] Correctly dismiss user prompts in MarionetteTestharnessProtocolPart._close_windows(). r?jgraham
47 bytes, text/x-phabricator-request
|Details | Review|
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
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.
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.
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())
Not really, given that in case of a user prompt being open, it will close it but not the window.
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.
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.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: -- → P2
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.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/fcb026c5194b [wpt] Correctly dismiss user prompts in MarionetteTestharnessProtocolPart._close_windows(). r=jgraham
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14737 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
You need to log in before you can comment on or make changes to this bug.