Bug 1590682 Comment 12 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

Review of attachment 9108308 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!  Thanks for the summary of the changes.  They look good to me, with some minor comments below.  Have not tested, but I'm fine with leaving that to the try run.  Will be great  to have these tests passing reliably, and to move further away from mozmill.

::: calendar/test/modules/ItemEditingHelpers.jsm
@@ +82,5 @@
>  `;
>  
> +function sleep(window) {
> +  return new Promise(resolve => window.setTimeout(resolve));
> +}

Nice, that is useful.  Should we export this for use in other test files?  This pattern came up a lot in the mochitests I've worked on recently

@@ +198,5 @@
>   *                      attendees.remove - eMail of attendees to remove, comma separated.
>   */
> +async function setData(dialog, iframe, data) {
> +  function replaceText(input, text) {
> +    synthesizeMouseAtCenter(input.getNode(), {}, iframe.window);

Huh, does the `{}` send a single click to the input?  Looks like it must, but you wouldn't know that just from looking at it.

@@ +446,5 @@
>  /**
>   * Set the categories in the event-dialog menulist-panel.
>   *
> + * @param iframeWindow    The event dialog iframe.
> + * @param index           Array containing the categories as strings - leave empty to clear.

The param doc 'index' doesn't match the param 'categories' here.

@@ +454,5 @@
> +  let menulist = iframeDocument.getElementById("item-categories");
> +  let menupopup = iframeDocument.getElementById("item-categories-popup");
> +
> +  synthesizeMouseAtCenter(menulist, {}, iframeWindow);
> +  await BrowserTestUtils.waitForEvent(menupopup, "popupshown");

A general question: In cases like this, is it guaranteed that the popupshown event won't fire before we are awaiting for it to fire?  Or could it fire really quickly after the click before we're waiting on it? Or...

@@ +467,5 @@
>    }
> +
> +  let hiddenPromise = BrowserTestUtils.waitForEvent(menupopup, "popuphidden");
> +  menupopup.hidePopup();
> +  await hiddenPromise;

...or should we use this approach of setting up the promise first, then awaiting it after doing the thing that causes it to resolve?

@@ +480,5 @@
> +async function handleAddingAttachment(dialogWindow, url) {
> +  let dialogDocument = dialogWindow.document;
> +
> +  synthesizeMouseAtCenter(dialogDocument.getElementById("button-url"), {}, dialogWindow);
> +  await BrowserTestUtils.promiseAlertDialog(undefined, undefined, attachment => {

"attachment" -> "attachmentWindow" or similar?

@@ +584,3 @@
>  
> +  if (!BrowserTestUtils.is_visible(label)) {
> +    menuitem.click();

.click() here instead of synthesizeMouseAtCenter ?  Probably fine, just curious since elsewhere you're using the latter to send clicks.

@@ +614,5 @@
> +      );
> +    }
> +  );
> +
> +  await new Promise(resolve => iframeWindow.setTimeout(resolve, 500));

Hm, I thought eslint was outlawing such setTimeouts now?

::: mail/test/resources/mozmill/mozmill/extension/content/stdlib/EventUtils.jsm
@@ +142,5 @@
>      }
>    }
>  }
>  
> +// Call synthesizeMouse with coordinates at the center of aTarget.

Better to use the `/** ... */` style comments for function doc string, for consistency.
Review of attachment 9108308 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!  Thanks for the summary of the changes.  They look good to me, with some minor comments to address.  Have not tested, but I'm fine with leaving that to the try run.  Will be great  to have these tests passing reliably, and to move further away from mozmill.

::: calendar/test/modules/ItemEditingHelpers.jsm
@@ +82,5 @@
>  `;
>  
> +function sleep(window) {
> +  return new Promise(resolve => window.setTimeout(resolve));
> +}

Nice, that is useful.  Should we export this for use in other test files?  This pattern came up a lot in the mochitests I've worked on recently

@@ +198,5 @@
>   *                      attendees.remove - eMail of attendees to remove, comma separated.
>   */
> +async function setData(dialog, iframe, data) {
> +  function replaceText(input, text) {
> +    synthesizeMouseAtCenter(input.getNode(), {}, iframe.window);

Huh, does the `{}` send a single click to the input?  Looks like it must, but you wouldn't know that just from looking at it.

@@ +446,5 @@
>  /**
>   * Set the categories in the event-dialog menulist-panel.
>   *
> + * @param iframeWindow    The event dialog iframe.
> + * @param index           Array containing the categories as strings - leave empty to clear.

The param doc 'index' doesn't match the param 'categories' here.

@@ +454,5 @@
> +  let menulist = iframeDocument.getElementById("item-categories");
> +  let menupopup = iframeDocument.getElementById("item-categories-popup");
> +
> +  synthesizeMouseAtCenter(menulist, {}, iframeWindow);
> +  await BrowserTestUtils.waitForEvent(menupopup, "popupshown");

A general question: In cases like this, is it guaranteed that the popupshown event won't fire before we are awaiting for it to fire?  Or could it fire really quickly after the click before we're waiting on it? Or...

@@ +467,5 @@
>    }
> +
> +  let hiddenPromise = BrowserTestUtils.waitForEvent(menupopup, "popuphidden");
> +  menupopup.hidePopup();
> +  await hiddenPromise;

...or should we use this approach of setting up the promise first, then awaiting it after doing the thing that causes it to resolve?

@@ +480,5 @@
> +async function handleAddingAttachment(dialogWindow, url) {
> +  let dialogDocument = dialogWindow.document;
> +
> +  synthesizeMouseAtCenter(dialogDocument.getElementById("button-url"), {}, dialogWindow);
> +  await BrowserTestUtils.promiseAlertDialog(undefined, undefined, attachment => {

"attachment" -> "attachmentWindow" or similar?

@@ +584,3 @@
>  
> +  if (!BrowserTestUtils.is_visible(label)) {
> +    menuitem.click();

.click() here instead of synthesizeMouseAtCenter ?  Probably fine, just curious since elsewhere you're using the latter to send clicks.

@@ +614,5 @@
> +      );
> +    }
> +  );
> +
> +  await new Promise(resolve => iframeWindow.setTimeout(resolve, 500));

Hm, I thought eslint was outlawing such setTimeouts now?

::: mail/test/resources/mozmill/mozmill/extension/content/stdlib/EventUtils.jsm
@@ +142,5 @@
>      }
>    }
>  }
>  
> +// Call synthesizeMouse with coordinates at the center of aTarget.

Better to use the `/** ... */` style comments for function doc string, for consistency.

Back to Bug 1590682 Comment 12