Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

The Marionette server is missing the ‘unexpected alert open’ error from WebDriver and an assertion to go with it.

Note that this will be preparatory work for https://bugzilla.mozilla.org/show_bug.cgi?id=1279211 and https://bugzilla.mozilla.org/show_bug.cgi?id=1264259.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8860957 [details]
Bug 1359004 - Add preliminary support for unexpected alerts

https://reviewboard.mozilla.org/r/132966/#review136054

Nice changes which will now give clear errors if an alert is open when the next command will be executed.

::: testing/marionette/assert.js:158
(Diff revision 2)
> + *     Custom error message.
> + *
> + * @throws {UnexpectedAlertOpen}
> + *     If there is a user prompt.
> + */
> +assert.noUserPrompt = function (dialog, msg = "") {

The exceptions are referencing `alerts` while you are using `user prompt` here. Shouldn't we try to be in sync?

::: testing/marionette/assert.js:160
(Diff revision 2)
> + * @throws {UnexpectedAlertOpen}
> + *     If there is a user prompt.
> + */
> +assert.noUserPrompt = function (dialog, msg = "") {
> +  const promptMissing = d => d === null || typeof d == "undefined";
> +  assert.that(promptMissing, msg, UnexpectedAlertOpenError)(dialog);

It would read easier when the callback is inline.

::: testing/marionette/driver.js:980
(Diff revision 2)
> + * @throws {UnsupportedOperationError}
> + *     Not available in current context.
> + * @throws {NoSuchWindowError}
> + *     Top-level browsing context has been discarded.
> + * @throws {UnexpectedAlertOpenError}
> + *     A modal dialog was open, blocking this operation.

The dialog is still open. I don't see that we are closing it anywhere. So can you do a s/was/is/ across the file?

::: testing/marionette/driver.js:1195
(Diff revision 2)
>  
>    yield goForward;
>  };
>  
>  /**
> - * Causes the browser to reload the page in in current top-level browsing context.
> + * Causes the browser to reload the page in in current top-level browsing

`in in`

::: testing/marionette/driver.js:1338
(Diff revision 2)
> + *     A modal dialog was open, blocking this operation.
>   */
>  GeckoDriver.prototype.getWindowRect = function (cmd, resp) {
> -  let win = assert.window(this.getCurrentWindow());
> +  const win = assert.window(this.getCurrentWindow());
> +  assert.noUserPrompt(this.dialog);
>    return {

nit: nearly all methods have an empty line after the asserts.

::: testing/marionette/driver.js:1546
(Diff revision 2)
> + *     A modal dialog was open, blocking this operation.
> + */
> +GeckoDriver.prototype.switchToParentFrame = function* (cmd, resp) {
>    assert.window(this.getCurrentWindow());
> -
> -  let res = yield this.listener.switchToParentFrame();
> +  assert.noUserPrompt(this.dialog);
> +  yield this.listener.switchToParentFrame();

nit: adding an empty line

::: testing/marionette/driver.js:1777
(Diff revision 2)
>  };
>  
>  /**
>   * Perform a series of grouped actions at the specified points in time.
>   *
>   * @param {Array.<?>} actions

Here is something mozreview doesn't seem to like. Not sure why red borders are getting drawn here. Can you check that all is fine?

::: testing/marionette/driver.js
(Diff revision 2)
> +  assert.noUserPrompt(this.dialog);
>  
> -  switch (this.context) {
> -    case Context.CHROME:
> +  let actions = cmd.parameters.actions;
> +  yield this.listener.performActions({"actions": actions});
> -      throw new UnsupportedOperationError(
> -          "Command 'performActions' is not yet available in chrome context");

The separation is here because we still have to implement the code for chrome scope.

I think it would be good to point out code which is not allowed to run in the current context vs. code which is not available yet.

The same applies to other changes in this file.

::: testing/marionette/error.js:503
(Diff revision 2)
>    ["session not created", SessionNotCreatedError],
>    ["stale element reference", StaleElementReferenceError],
>    ["timeout", TimeoutError],
>    ["unable to set cookie", UnableToSetCookieError],
>    ["unknown command", UnknownCommandError],
> +  ["unexpected alert open", UnexpectedAlertOpenError],

Lift up by one line so we keep the alphabetical order.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:59
(Diff revision 2)
>  
>      def test_no_alert_raises(self):
> -        self.assertRaises(NoAlertPresentException, Alert(self.marionette).accept)
> -        self.assertRaises(NoAlertPresentException, Alert(self.marionette).dismiss)
> +        with self.assertRaises(errors.NoAlertPresentException):
> +            Alert(self.marionette).accept()
> +        with self.assertRaises(errors.NoAlertPresentException):
> +            Alert(self.marionette).dismiss()

Wow, the former code was really faulty.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 4 • 7 years ago
mozreview-review-reply

Comment on attachment 8860957 [details]
Bug 1359004 - Add preliminary support for unexpected alerts

https://reviewboard.mozilla.org/r/132966/#review136054

> The exceptions are referencing `alerts` while you are using `user prompt` here. Shouldn't we try to be in sync?

The error message mandated by Selenium is ‘unexpected alert open’, but this is mostly for backwards compatibility reasons with Selenium.  The WebDriver standard talks about ‘user prompts’, because that is the official term used by HTML.

However, I do notice that this once case uses "UnexpectedAlertOpen".  It should be "UnexpectedAlertOpenError".

> It would read easier when the callback is inline.

OK, if you think so.

> `in in`

I copied this from the specification, so you inadvertently found a spec bug too: https://github.com/w3c/webdriver/issues/902

> Here is something mozreview doesn't seem to like. Not sure why red borders are getting drawn here. Can you check that all is fine?

I suspect the syntax highlighter used by mozreview doesn’t like "<?" to appear inline in comments.

> The separation is here because we still have to implement the code for chrome scope.
> 
> I think it would be good to point out code which is not allowed to run in the current context vs. code which is not available yet.
> 
> The same applies to other changes in this file.

I only fixed performActions and getActiveElement.  We will never implement multiAction, since that belongs to the legacyaction.js module and is scheduled for removal.

	Pulsebot
	Comment 6 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/031223765720
Add preliminary support for unexpected alerts r=whimboo

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 7 • 7 years ago
mozreview-review-reply

Comment on attachment 8860957 [details]
Bug 1359004 - Add preliminary support for unexpected alerts

https://reviewboard.mozilla.org/r/132966/#review136054

> I only fixed performActions and getActiveElement.  We will never implement multiAction, since that belongs to the legacyaction.js module and is scheduled for removal.

I see. Thanks.

	Carsten Book [:Tomcat]
	Comment 8 • 7 years ago
bugherder

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