1332708 - Mozmill failure on 20-01-2017: Timeouts and utils.waitFor() failing

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jorg K (CEST = GMT+2)]

Sadly Mozmill is currently red due to bug 1329288, so additional bustage is hard to see.

Last good:
Thu Jan 19, 2016, 18:53:22

First bad:
Fri Jan 20, 2016, 8:08:48
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=2d4babd80f8669f3bea94c87edf4dbf6da5d0d12

TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cal-recurrence\testAnnualRecurrence.js | testAnnualRecurrence.js::testAnnualRecurrence
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cal-recurrence\testBiweeklyRecurrence.js | testBiweeklyRecurrence.js::testBiweeklyRecurrence
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cal-recurrence\testDailyRecurrence.js | testDailyRecurrence.js::testDailyRecurrence
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cal-recurrence\testLastDayOfMonthRecurrence.js | testLastDayOfMonthRecurrence.js::testLastDayOfMonthRecurrence
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cal-recurrence\testWeeklyNRecurrence.js | testWeeklyNRecurrence.js::testWeeklyNRecurrence
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cal-recurrence\testWeeklyUntilRecurrence.js | testWeeklyUntilRecurrence.js::testWeeklyUntilRecurrence
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cal-recurrence\testWeeklyWithExceptionRecurrence.js | testWeeklyWithExceptionRecurrence.js::testWeeklyWithExceptionRecurrence

TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\testLocalICS.js | testLocalICS.js::testLocalICS

What's going on here, MMD, can you please take a look.

Comment 1

[Jorg K (CEST = GMT+2)]

There is also a failure in test-header-toolbar.js. Log says:
INFO -  SUMMARY-UNEXPECTED-FAIL | test-header-toolbar.js | test-header-toolbar.js::test_customize_header_toolbar_change_button_style
INFO -    EXCEPTION: Timeout waiting for window to close!

Some of the calendar failures say:
EXCEPTION: waitFor: Timeout exceeded for '() => mozmill.utils.getWindows("Calendar:EventDialog").length == 0'

So it looks like M-C changed some window handling.

Regression range M-C
Good: a3978751f45108ff1ae002ecebdc0fa23f
Bad:  aa3e49299a3aa5cb0db570532e3df9e75d

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a3978751f45108ff1ae002ecebdc0fa23f&tochange=aa3e49299a3aa5cb0db570532e3df9e75d

Comment 2

[Jorg K (CEST = GMT+2)]

mozmake SOLO_TEST=message-header/test-header-toolbar.js mozmill-one
fails locally with:

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\message-header\test-header-toolbar.js | test-header-toolbar.js::test_customize_header_toolbar_change_button_style
  EXCEPTION: Timeout waiting for window to close!
    at: utils.js line 447
       TimeoutError utils.js:447 13
       waitFor utils.js:485 11
       WindowWatcher_waitForWindowClose test-window-helpers.js:398 5
       wait_for_window_close test-window-helpers.js:639 3
       CustomizeDialogHelper_close test-customization-helpers.js:83 7
       test_customize_header_toolbar_change_button_style test-header-toolbar.js:408 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestFile frame.js:534 3
       runTestFile frame.js:713 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3

Aceman, you have a lot of experience with this. Could you check the regression range and give and/or give me a tip?

Comment 3

[Jorg K (CEST = GMT+2)]

Hmm, test-header-toolbar.js:408 is |gCDHelper.close(ctc);|.

Let's see the calendar failures:
Four have this failure:
test-calendar-utils.js:328 which is
  controller.waitFor(() => mozmill.utils.getWindows("Calendar:EventDialog").length == 0);

Twelve failures have this
MozMillController.prototype.waitFor controller.js:687 which is
  utils.waitFor(callback, message, timeout, interval, thisObject);

And one has WindowWatcher_waitForWindowClose test-window-helpers.js:398 which is
  utils.waitFor(() => this.monitorizeClose()

So something is is wrong with waiting, not sure if it's only waiting for windows to close.

Comment 4

[[:MakeMyDay]]

There have been a few of that timeout issues before (see comment in bug 980515) already, but that haven't been as massive as now. Markus, can you take a look at the calendar failures?

Comment 5

[:aceman]

I see that on Linux (32bit) the failure in
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cal-recurrence/testWeeklyWithExceptionRecurrence.js | testWeeklyWithExceptionRecurrence.js::testWeeklyWithExceptionRecurrence
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cal-recurrence/testWeeklyUntilRecurrence.js | testWeeklyUntilRecurrence.js::testWeeklyUntilRecurrence

actually started since bug 980515 on Jan 7.

So just for the experiment, have you tried pushing revert of bug 1332380 if that helps?
Or the m-c version of it, bug 1312216.

Comment 6

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #5)
> 
> So just for the experiment, have you tried pushing revert of bug 1332380 if
> that helps?
> Or the m-c version of it, bug 1312216.

This are changes in the in-content prefs only. So I think this will have no influence here.

Comment 7

[:aceman]

So the test fails waiting for something (e.g. at https://dxr.mozilla.org/comm-central/rev/9cb95ca4c3f7cbd473e78addb8e3607af6af8386/calendar/test/mozmill/shared-modules/test-calendar-utils.js#328) after some event.
What about https://hg.mozilla.org/mozilla-central/rev/5a03c87a1725 ?

Comment 8

[Jorg K (CEST = GMT+2)]

M-C rev 5a03c87a1725 couldn't be backed out easily, so I backed out

https://hg.mozilla.org/mozilla-central/rev/30e79c2a8338
https://hg.mozilla.org/mozilla-central/rev/8a0dc12ec71f and
https://hg.mozilla.org/mozilla-central/rev/5a03c87a1725, see:
https://hg.mozilla.org/mozilla-central/filelog/3cedab21a7e65e6a1c4c2294ecfb5502575a46e3/toolkit/modules/SelectParentHelper.jsm

That partially backs out two bugs: bug 1321472 and bug 1311279 which is most likely not a great idea since these two bugs have six changesets in total.

Anyway, with these changes backed out this
mozmake SOLO_TEST=message-header/test-header-toolbar.js mozmill-one
passes again.

Neil and Mike, can you please lend a hand here. What do we need to do to get our tests going again?

Comment 9

[Markus Adrario [:Taraman]]

The failures all seem to happen within the event-Dialog.
< controller.waitFor(() => mozmill.utils.getWindows("Calendar:EventDialog").length == 0);
This is the part, where we wait for the Dialog to close.

I'll try to reproduce it locally to see what happens.

Comment 10

[Jorg K (CEST = GMT+2)]

Markus: I wouldn't spend to much time on it. Something has changed in M-C, see my previous comment #8, and that either needs to be fixed on M-C or in the C-C Mozmill framework.

Comment 11

[Neil Deakin]

What happens if you just delete the parts I added to popup.xml in https://hg.mozilla.org/mozilla-central/rev/30e79c2a8338 and leave everything else as is?

What happens if you leave every patch as is but just comment out the line that calls 'setCaptureAlways()'?

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Neil Deakin from comment #11)
> What happens if you just delete the parts I added to popup.xml in
> https://hg.mozilla.org/mozilla-central/rev/30e79c2a8338 and leave everything
> else as is?
Removing the code in the
<implementation> and <handlers> tags at the end of the file and therefore reducing that section to

  <binding id="popup-scrollbars" extends="chrome://global/content/bindings/popup.xml#popup">
    <content>
      <xul:scrollbox class="popup-internal-box" flex="1" orient="vertical" style="overflow: auto;">
        <children/>
      </xul:scrollbox>
    </content>
  </binding>

makes the TB Mozmill test pass. I I haven't tried the Calendar ones.

> What happens if you leave every patch as is but just comment out the line
> that calls 'setCaptureAlways()'?
Putting the removed section back (hg revert --all) and just taking out |this.setCaptureAlways();|, thus changing the section to:

  <![CDATA[
    if (!this._draggingState) {
      this._draggingState = overItem ? this.DRAG_OVER_POPUP : this.DRAG_OVER_BUTTON;
    }
  ]]>

also makes my test pass.

I will ship off a try run of C-C with this one line removed from M-C (I can do that ;-)).

Thanks for your help!

Comment 13

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=309695d08c95c81f613e51f7416f82242e512dc1

Fingers crossed. Now, if that fixes the other Mozmill failures as well, what is the way forward?

Comment 14

[Jorg K (CEST = GMT+2)]

Removing |this.setCaptureAlways();| fixed all the Mozmill failures reported in this bug. The "Z" is still red due to a failure from bug 1329288 (which will be fixed soon).

So what's the way forward?

Comment 15

[Neil Deakin]

Attachment: dragonlywhenopen

The issue is that these tests are trying to click on a menuitem while the menulist isn't open and the mouse capture is getting confused. This patch doesn't enable mouse capture when the popup isn't open.

The test, of course, should really be waiting for the menulist's popup to open first if it is trying to click on items in it.

Comment 16

[:aceman]

You mean the TB tests? We can fix those if needed.

Comment 17

[:aceman]

It seems to me in test-header-toolbar.js::test_customize_header_toolbar_change_button_style() we need to click(<menulist id="modelist">) before clicking the iconMode or textMode buttons.

Comment 18

[:aceman]

For the calendar tests, at https://dxr.mozilla.org/comm-central/rev/9cb95ca4c3f7cbd473e78addb8e3607af6af8386/calendar/test/mozmill/shared-modules/test-calendar-utils.js# we should wait for the menulist to open, like:
aController.waitFor(() => menulist.menupopup.state == "open", "menupoup didn't open")

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #17)
Please suggest the exact line you want me to add and it's location.

(In reply to :aceman from comment #18)
Where exactly would you like me to insert this line?

I'm in favour of fixing the test *now* before the M-C change goes ahead, preferably today before the branch day tomorrow.

Comment 20

[:aceman]

(In reply to Jorg K (GMT+1) from comment #19)
> (In reply to :aceman from comment #17)
> Please suggest the exact line you want me to add and it's location.

https://dxr.mozilla.org/comm-central/rev/9cb95ca4c3f7cbd473e78addb8e3607af6af8386/mail/test/mozmill/message-header/test-header-toolbar.js#405 :

ctc.click(new elib.eid("modelist"));
ctc.waitFor(() => ctc.window.document.getElementById("modelist").menupopup.state == "open", "menupoup didn't open");

The same at line 416.
 
> (In reply to :aceman from comment #18)
> Where exactly would you like me to insert this line?

https://dxr.mozilla.org/comm-central/rev/9cb95ca4c3f7cbd473e78addb8e3607af6af8386/calendar/test/mozmill/shared-modules/test-calendar-utils.js#861

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1332708-fix-mozmill.patch for C-C

Like this? I can't try it right now since my machine is blocked for another hour by yet another recompile after yet another bustage fix.

Comment 22

[Jorg K (CEST = GMT+2)]

BTW, what's "modelist"?

Comment 23

[:aceman]

Comment on attachment 8829201 [details] [diff] [review]
1332708-fix-mozmill.patch for C-C

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

Yes, I think that is the missing logic.
Maybe the 'new' before elib.eid() is not needed, or it will need to be elib.Elem(ctc.window.document.getElementById("")) so you may need to play with the syntax locally.

Comment 24

[:aceman]

(In reply to Jorg K (GMT+1) from comment #22)
> BTW, what's "modelist"?

As said in comment 17, just the id of the menulist element we want to open first before operating its menuitems.
https://dxr.mozilla.org/comm-central/rev/a3978751f45108ff1ae002ecebdc0fa23fc52b84/mozilla/toolkit/content/customizeToolbar.xul#45

Comment 25

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=067f3844cc6ccaf0e65442784820416ab880f74c

Comment 26

[Jorg K (CEST = GMT+2)]

OK, now the calendar tests all fail with:
  EXCEPTION: menupoup didn't open (11 times seen in the file).

And test_customize_header_toolbar_change_button_style says:
  EXCEPTION: elib.eid is not a constructor

So not a total success. Sadly my local build broken, so I have to fix that first.

Comment 27

[:aceman]

(In reply to Jorg K (GMT+1) from comment #26)
> OK, now the calendar tests all fail with:
>   EXCEPTION: menupoup didn't open (11 times seen in the file).

Not sure what is the problem here, try
+    aController.waitFor(() => menulist.menupopup
+                                      .state == "open", "menupopup didn't open, state="+menulist.menupopup.state);
 
> And test_customize_header_toolbar_change_button_style says:
>   EXCEPTION: elib.eid is not a constructor
> 

Try without the 'new'.

Comment 28

[Jorg K (CEST = GMT+2)]

I will, thanks, my local build is still broken, I'm doing the fourth full build today. Maybe it was not working due to all the XPCOM glue changes and the fact that I had config option "disable sandboxing" set. Grrr.

.menupop is the right thing to do? I haven't see this elsewhere.

Comment 29

[:aceman]

(In reply to Jorg K (GMT+1) from comment #28)
> .menupop is the right thing to do? I haven't see this elsewhere.

I like it :) There wasn't an error about it. If should fetch the menupopup child of the menulist. Other way is to find the menupopup element via ID (if it has one) so it seems the first method is more robust (if we already have the parent menulist found).

Comment 30

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #27)
> Try without the 'new'.
OK, I can work again. Without the 'new' I get:
  EXCEPTION: elib.eid is not a function

Comment 31

[Jorg K (CEST = GMT+2)]

Next I tried:

  let ctc = gCDHelper.open(mc);
  let popup = ctc.window.document.getElementById("modelist");
  ctc.click(new elib.Elem(popup));
  ctc.waitFor(() => popup.state == "open", "menupoup didn't open, state="+popup.state);
  let iconMode = ctc.window.document.getElementById("main-box").
    querySelector("[value*='icons']");
  ctc.click(new elib.Elem(iconMode));
  gCDHelper.close(ctc);

Now I get the same as in calendar:
  EXCEPTION: menupoup didn't open, state=undefined

Watching the test, the popup does show, but no item is clicked on it.

I also tried

  // Change the button style to icon (only) mode
  let ctc = gCDHelper.open(mc);
  let popup = ctc.window.document.getElementById("modelistpopup");
  ctc.click(new elib.Elem(popup));
  ctc.waitFor(() => popup.state == "open", "menupoup didn't open, state="+popup.state);
  let iconMode = ctc.window.document.getElementById("main-box").
    querySelector("[value*='icons']");
  ctc.click(new elib.Elem(iconMode));
  gCDHelper.close(ctc);

and that gave: EXCEPTION: menupoup didn't open, state=closed

Comment 32

[Jorg K (CEST = GMT+2)]

Next attempt:

  // Change the button style to icon (only) mode
  let ctc = gCDHelper.open(mc);
  let list = ctc.window.document.getElementById("modelist");
  let popup = ctc.window.document.getElementById("modelistpopup");
  ctc.click(new elib.Elem(list));
  ctc.waitFor(() => popup.state == "open", "menupoup didn't open, state="+popup.state);
  let iconMode = ctc.window.document.getElementById("main-box").
    querySelector("[value*='icons']");
  ctc.click(new elib.Elem(iconMode));
  gCDHelper.close(ctc);

That fails on the .close(ctc).

I don't actually understand what we're trying to click on.

https://dxr.mozilla.org/comm-central/rev/a3978751f45108ff1ae002ecebdc0fa23fc52b84/mozilla/toolkit/content/customizeToolbar.xul#45

Both modelist and modeicons have value icons, so which one does the query selector select?? And what's the value*?

Comment 33

[Jorg K (CEST = GMT+2)]

Next attempt:

  // Change the button style to icon (only) mode
  let ctc = gCDHelper.open(mc);
  let list = ctc.window.document.getElementById("modelist");
  let popup = ctc.window.document.getElementById("modelistpopup");
  let icons = ctc.window.document.getElementById("modeicons");
  ctc.click(new elib.Elem(list));
  ctc.waitFor(() => popup.state == "open", "menupoup didn't open, state="+popup.state);
  ctc.click(new elib.Elem(icons));
  gCDHelper.close(ctc);

The menu clearly shows, but the click on items doesn't happen, so the .close fails.

What does the .open(() actually open?

I just remove the .close() calls from the original code and the test runs though and the fails after restoreDefaultButtons().

Comment 34

[Jorg K (CEST = GMT+2)]

Last attempt:

  // Change the button style to icon (only) mode
  let ctc = gCDHelper.open(mc);
  let list = ctc.window.document.getElementById("modelist");
  let popup = ctc.window.document.getElementById("modelistpopup");
  ctc.click(new elib.Elem(list));
  ctc.waitFor(() => popup.state == "open", "menupoup didn't open, state="+popup.state);

  let icons = ctc.window.document.getElementById("modeicons");
  ctc.click(new elib.Elem(icons));
  ctc.waitFor(() => popup.state == "closed", "menupoup didn't close, state="+popup.state);
  gCDHelper.close(ctc);

Gives:
  EXCEPTION: menupoup didn't close, state=open

So the menu opens, but as much as you click, the menu stays open.

I think I should give up since I obviously don't understand what
- .open() should do
- .close(0 should do
- why the click doesn't have any effect on a menu that's visibly open.
- why the whole thing words by just removing the .close() but then fails later.

Comment 35

[:aceman]

Ah sorry, .eid() is on the controller, so try ctc.click(ctc.eid("modelist"))

Both of the attempts in comment 31 have bugs that is why they didn't work. But thanks for trying. The version in comment 32 seems workable (just verbose).

It seems to me it wants to click the item with id="modeicons" it just finds it via having the right value attribute (I didn't check what the * does in the selector).

What do you mean it fails on .close(ctc)? What is the error?

Comment 36

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #35)
> What do you mean it fails on .close(ctc)? What is the error?
EXCEPTION: Timeout waiting for window to close!

Sorry, I can't find any working permutation as much as I have tried.

The popup opens and the selected "base" value is icons, but the selected popup value is "Icons and Text". An can even manually click in the popup, but the popup doesn't close. In the end it times out.

So summary: We have no problem opening the popup, but it just never closes.

Comment 37

[:aceman]

I assume the .open() and .close() are methods to open and close the customization palette (dialog).
https://dxr.mozilla.org/comm-central/rev/9cb95ca4c3f7cbd473e78addb8e3607af6af8386/mail/test/mozmill/shared-modules/test-customization-helpers.js#47

Thus the test opens the dialog, clicks a menulist in it, then an item in the menu. Then it needs to close the customization dialog. This repeats then. If it fails to close the dialog (or you removed the call) it may backfire later in the test.

Comment 38

[Jorg K (CEST = GMT+2)]

Yet another attempt:

  // Change the button style to icon (only) mode
  let ctc = gCDHelper.open(mc);
  let iconMode = ctc.window.document.getElementById("main-box").
    querySelector("[value*='icons']");
  ctc.click(ctc.eid("modelist"));
  ctc.waitFor(() => ctc.window.document.getElementById("modelist")
                       .menupopup.state == "open", "menupoup didn't open");
  ctc.click(new elib.Elem(iconMode));
  gCDHelper.close(ctc);

Fails on the .close() with error
  EXCEPTION: Timeout waiting for window to close!
I repeat: Clicking on the icons item, selects the base value, but never changes the value in the popup which remains "Icons and Text". The popup also doesn't close. I have the impression that something is broken there.

We do all the right things. We click on the list, we check that the state is open, we supposedly click in an open popup, which is visually open, but it never closes. Look at all the variations in the previous comments. Nothing works, well, it's basically all the same, just addressing the different element differently.

Comment 39

[Jorg K (CEST = GMT+2)]

Wires crossed with comment #37. So if .close() wants to close the whole dialogue, that thoroughly fails since the popup menu is still up. And it doesn't respond to clicks, not even manual ones.

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: Picture showing the hung menu

Neil, as you can see from the flood of comments, I've spent a long time with this and couldn't find a working solution.

You said in comment #15:
> The test, of course, should really be waiting for the menulist's 
> popup to open first if it is trying to click on items in it.

So I modified the code to do this:

  // Change the button style to icon (only) mode
  let ctc = gCDHelper.open(mc);
  let iconMode = ctc.window.document.getElementById("main-box").
    querySelector("[value*='icons']");
  ctc.click(ctc.eid("modelist"));
  ctc.waitFor(() => ctc.window.document.getElementById("modelist")
                       .menupopup.state == "open", "menupoup didn't open");
  dump(ctc.window.document.getElementById("modelist").value+" <== before click\n");
  ctc.click(new elib.Elem(iconMode));
  dump(ctc.window.document.getElementById("modelist").value+" <== after click\n");
  gCDHelper.close(ctc);  <=== 413

The menu is defined here:
https://dxr.mozilla.org/mozilla-central/rev/3cedab21a7e65e6a1c4c2294ecfb5502575a46e3/toolkit/content/customizeToolbar.xul#45
yet, TB adds another time "Text beside Icons" here:
https://dxr.mozilla.org/comm-central/rev/9cb95ca4c3f7cbd473e78addb8e3607af6af8386/mail/base/content/mailCore.js#77

What I see in the test is what you can see in the attached picture. The popup opens and the original value is highlighted "Icons beside Text". Then the click is performed, and the value changes to "icons" as the debug produced by the code above shows:

textbesideicon <== before click
icons <== after click

The popup never closes and in the end the test fails with
  EXCEPTION: Timeout waiting for window to close!
at test_customize_header_toolbar_change_button_style test-header-toolbar.js:413 which is the line I marked.

As you can see in comment #34, I had a version like
  let popup = ctc.window.document.getElementById("modelistpopup");
  ...
  let icons = ctc.window.document.getElementById("modeicons");
  ctc.click(new elib.Elem(icons));
  ctc.waitFor(() => popup.state == "closed", "menupoup didn't close, state="+popup.state);
  gCDHelper.close(ctc);

and that gave:
  EXCEPTION: menupoup didn't close, state=open

So while we're happy to fix our tests where they do thing they shouldn't be doing, like simulating clicking on closed menu items, I have the impression that despite our best efforts, there is a bug in the M-C code.

At the end, I tried your patch with the our modified test and our original test. The modified version which visibly brings up the popup menu still fails, but the original test which clicks on an item in a non-visible popup succeeds.

So did I just spent time in vain trying to improve the tests?

Comment 41

[Neil Deakin]

I'm pretty sure element.click() doesn't synthesize a mouse event. It just sends the mouse events to the dom element, and does no default handling.

Did you try test patch I posted?

Comment 42

[Jorg K (CEST = GMT+2)]

Oops, my comment was so long that you didn't read to the end ;-(
Let me repeat the end of comment #40:

===

At the end, I tried your patch with the our modified test and our original test. The modified version which visibly brings up the popup menu still fails, but the original test which clicks on an item in a non-visible popup succeeds.

So did I just spent time in vain trying to improve the tests?

===

My aim was to fix the tests so that they pass without your patch, and I failed.

With your patch, the original tests pass. But didn't you tell us we should really open the popup first?

Comment 43

[Neil Deakin]

We would want my patch regardless.

I don't know what the test is trying to test, so I can't really answer exactly. If it is trying to verify the contents and behaviour of the menu and/or could be affected by other UI it should use lower level mouse synthesis. If it is just trying to test the code that would run when the menuitem is selected runs, then the test would probably be ok as is.

Comment 44

[:aceman]

Neil, Jorg was trying the other test, where we NOW do proper controller.click(menulist element), see comment 40.

The strange thing is all other menulists in TB seem to work fine (tests pass), just the 2 menulists in the failed tests (calendar something and one in the customize dialog).

Comment 45

[Jorg K (CEST = GMT+2)]

(In reply to Neil Deakin from comment #43)
> If it is just trying to test the code that would run when the
> menuitem is selected runs, then the test would probably be ok as is.
Yes, that's the aim. Run something from a menu and check the result.

(In reply to :aceman from comment #44)
> Neil, Jorg was trying the other test, where we NOW do proper
> controller.click(menulist element), see comment 40.
element.click() (comment #41) or controller.click(menulist element), I'm confused now.

Comment 46

[:aceman]

(In reply to Jorg K (GMT+1) from comment #45)
> (In reply to :aceman from comment #44)
> > Neil, Jorg was trying the other test, where we NOW do proper
> > controller.click(menulist element), see comment 40.
> element.click() (comment #41) or controller.click(menulist element), I'm
> confused now.

We played with controller.click(menulist). The failing calendar tests use menulist.click() which Neil thinks may not be as great. We can rewrite that too if needed.

Comment 47

[Jorg K (CEST = GMT+2)]

Neil, could you please get this reviewed, landed and uplifted to M-A. Our C-C and C-A trees look quite red due to the Mozmill failures.

Sorry for "spamming" your bug with comments related to improving our tests. We can do this in a different bug later. Now my focus is on getting the trees green again.

Comment 48

[Jorg K (CEST = GMT+2)]

Green TB try run with this patch applied here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=74fc0c3a8846bb4a90fed8dccdbd57c0f25847ac

Comment 49

[Jorg K (CEST = GMT+2)]

Sorry to be a nuisance here, but can we please get this landed (and uplifted). Please just set "checkin-needed" if you don't want to push it yourself.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc004d33b0f9
open start dragging on a menuitem when dropdown is open, r=mconley

Comment 51

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 52

[Jorg K (CEST = GMT+2)]

Comment on attachment 8829185 [details] [diff] [review]
dragonlywhenopen

I'm requesting uplift here on behalf of Neil, who can provide further details.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1321472 landed on mozilla53
[User impact if declined]:
Firefox: Neil please fill in details
Thunderbird: Mozmill tests failing.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]:
Firefox: Neil please fill in details.
Thunderbird: Yes, with this fix Thunderbird's Mozmill tests pass now.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No small change, a missing bit from bug 1321472, covered by test.
[Why is the change risky/not risky?]: see above.
[String changes made/needed]: None.

As I understand comment #43 ("We would want my patch regardless.") correctly, this is also needed for Firefox. Neil can provide further details.

Comment 53

[:aceman]

Thanks Jorg for filing the mozmill bug for calendar. I'll file the other one.

Comment 54

[Neil Deakin]

This change fixes a correctness check, where one can start a pseudo-drag when the menu isn't open causing wierd UI behaviour. That said, it is unlikely to happen outside of tests, as is the case with this bug.

Comment 55

[Jorg K (CEST = GMT+2)]

Julien, could you please approve this. Our Mozmill tests look terrible on Aurora due to this bug:
https://treeherder.mozilla.org/#/jobs?repo=comm-aurora

Comment 56

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8829185 [details] [diff] [review]
dragonlywhenopen

Fixes a mozmill test, let's uplift to Aurora53

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63fb51734196