Closed Bug 1590682 Opened 4 months ago Closed 4 months ago

Try to improve the pass rate of event dialog tests

Categories

(Calendar :: Dialogs, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

Attachments

(3 files, 2 obsolete files)

The event dialog tests have been really bad lately. I want to start removing a lot of the Mozmill cruft, but first they have to start passing more reliably. This bug is for changes that attempt to fix them.

Attached patch 1590682-save-prompt.diff (obsolete) — Splinter Review

This moves the save prompt listener from browser_eventDialogModificationPrompt.js to the folder's head file, as the save prompt is turning up unexpectedly in other tests too. It's still considered a test failure, but it isn't wiping out the rest of the run by timing out.

Attachment #9103475 - Flags: review?(paul)
Attachment #9103475 - Flags: approval-calendar-beta?(paul)

This attempts to fix the "event-all-day could not be checked/unchecked" error, which doesn't seem to be accurate at all. I've skipped having the Mozmill controller check the state of the checkbox and got Mochitest to do so instead.

Attachment #9103477 - Flags: review?(paul)
Attachment #9103477 - Flags: approval-calendar-beta?(paul)

I still need to figure out why the save prompt is appearing, and I've seen one or two really weird stack traces in the error logs, so that should be interesting …

Comment on attachment 9103475 [details] [diff] [review]
1590682-save-prompt.diff

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

Looks good, assuming we don't have any tests where it's expected for this dialog to appear.  If we need that in the future we can just adapt then.

::: calendar/test/browser/eventDialog/head.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// If the "do you want to save the event?" prompt appears, this test failed.

Nit: this test failed -> the test failed  (Since there will be more than one test involved.)
Attachment #9103475 - Flags: review?(paul)
Attachment #9103475 - Flags: review+
Attachment #9103475 - Flags: approval-calendar-beta?(paul)
Attachment #9103475 - Flags: approval-calendar-beta+
Comment on attachment 9103477 [details] [diff] [review]
1590682-checkbox-failure.diff

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

Looks reasonable.  Hopefully this will clear up the error.
Attachment #9103477 - Flags: review?(paul)
Attachment #9103477 - Flags: review+
Attachment #9103477 - Flags: approval-calendar-beta?(paul)
Attachment #9103477 - Flags: approval-calendar-beta+

Nit fixed.

Attachment #9103816 - Flags: review+
Attachment #9103816 - Flags: approval-calendar-beta+
Attachment #9103475 - Attachment is obsolete: true
Keywords: checkin-needed

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/1e848c86da18
Listen for save event prompts on all event dialog tests; r=pmorris
https://hg.mozilla.org/comm-central/rev/3eca33a2f36b
Fix "event-all-day could not be checked/unchecked" failure; r=pmorris

Keywords: checkin-needed

I forgot this bug existed. Oh well, there's bug 1591829 now too.

You've got to set a milestone or the uplift will be missed.

Target Milestone: --- → 71
Attached patch 1590682-convert-setdata-1.diff (obsolete) — Splinter Review

Here's the rewrite all the annoying bits option I've been trying to avoid. It's just run 24 times on Try without a failure so hopefully we can call it a fix.

To summarise what I've done: invokeEventDialog and setData are now async functions and I've replaced much of the Mozmill controller-centric code with proper Mochitest code. I did use the Mozmill version of EventUtils because the other one isn't good to use in a JSM. I'll figure out a better solution for that one day so we don't have to maintain a second version.

Attachment #9108308 - Flags: review?(paul)
Attachment #9108308 - Flags: approval-calendar-beta?(paul)
Keywords: leave-open
Comment on attachment 9108308 [details] [diff] [review]
1590682-convert-setdata-1.diff

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.
Attachment #9108308 - Flags: review?(paul)
Attachment #9108308 - Flags: review+
Attachment #9108308 - Flags: approval-calendar-beta?(paul)
Attachment #9108308 - Flags: approval-calendar-beta+

(In reply to Paul Morris [:pmorris] from comment #12)

+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

It is a useful shorthand. I overuse timeouts, several of the ones in this patch are probably unnecessary.

As for exporting it, this probably isn't the best place to have it, and I especially don't want this file imported for any reason other than the existing ones. At some point I've got to do a grand rationalisation of a lot of Mochitest code, but it won't be until after the other Mozmill conversion is done.

If you want to copy these three lines into a head file for a directory of tests, that's probably the way to go for now.

+    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.

Yeah, it does. I hate the arguments of this function, especially since the second is non-optional, even without the third.

+  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...

+  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?

In the first case, we know the event won't happen until after the listener is added. The mouseup and mousedown events created by synthesizeMouseAtCenter are added to the end of a list of things to do. We're currently somewhere higher up the list, so this code will keep executing (and add the listener) until it runs out of things to do (in this case when it waits for a Promise that didn't immediately resolve) then do the next thing on the list.

Incidentally this is exactly what setTimeout with no (or 0) timeout does: it puts the callback on the end of the list. So we can use it to wait for things to happen, like custom elements or event handlers doing things.

In the second case, calling hidePopup fires the popuphidden event before returning. I'm pretty sure we don't need to wait for the event, but I've done it now. (I think at some point I was closing this menu pop-up a different way then got frustrated and did it this way.)

+  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.

I usually consider this to be cheating but it works. This menu item is part of the window's main menu which means it's a native menu item on Mac OS and we can't fire click events at it.

+  await new Promise(resolve => iframeWindow.setTimeout(resolve, 500));

Hm, I thought eslint was outlawing such setTimeouts now?

(I only remembered this was there after submitting the patch. I can't for the life of me work out why it's necessary but something is going on here.) I got away with it because this of where the file is. The path doesn't match any of the patterns for XPCShell or Mochitest files. TBH I'd probably just silence the linter warning here anyway. ;-)

Attachment #9108308 - Attachment is obsolete: true
Attachment #9108558 - Flags: review+
Attachment #9108558 - Flags: approval-calendar-beta+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/bb95517489e3
Convert troublesome parts of calendar tests to real mochitest code; r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.