Closed
Bug 1332708
Opened 8 years ago
Closed 8 years ago
Mozmill failure on 20-01-2017: Timeouts and utils.waitFor() failing
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox52 | --- | unaffected |
firefox53 | --- | fixed |
firefox54 | --- | fixed |
People
(Reporter: jorgk-bmo, Assigned: enndeakin)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
mconley
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details |
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.
Flags: needinfo?(makemyday)
Reporter | ||
Comment 1•8 years ago
|
||
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
Reporter | ||
Comment 2•8 years ago
|
||
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?
Flags: needinfo?(acelists)
Reporter | ||
Comment 3•8 years ago
|
||
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•8 years ago
|
||
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?
Flags: needinfo?(makemyday) → needinfo?(Mozilla)
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.
Flags: needinfo?(acelists)
Comment 6•8 years ago
|
||
(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.
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 ?
Reporter | ||
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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.
Flags: needinfo?(Mozilla)
Reporter | ||
Comment 10•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
Keywords: regression
Summary: Massive Mozmill failure in Calendar on 20-01-2017 → Mozmill failure on 20-01-2017: Timeouts and utils.waitFor() failing
Assignee | ||
Comment 11•8 years ago
|
||
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()'?
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 12•8 years ago
|
||
(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!
Flags: needinfo?(mconley)
Reporter | ||
Comment 13•8 years ago
|
||
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?
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 14•8 years ago
|
||
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?
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Flags: needinfo?(enndeakin)
Comment 16•8 years ago
|
||
You mean the TB tests? We can fix those if needed.
Comment 17•8 years ago
|
||
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•8 years ago
|
||
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")
Reporter | ||
Comment 19•8 years ago
|
||
(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.
Flags: needinfo?(acelists)
Comment 20•8 years ago
|
||
(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
Flags: needinfo?(acelists)
Reporter | ||
Comment 21•8 years ago
|
||
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.
Attachment #8829201 -
Flags: feedback?(acelists)
Reporter | ||
Comment 22•8 years ago
|
||
BTW, what's "modelist"?
Comment 23•8 years ago
|
||
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.
Attachment #8829201 -
Flags: feedback?(acelists) → feedback+
Comment 24•8 years ago
|
||
(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
Reporter | ||
Comment 25•8 years ago
|
||
Reporter | ||
Comment 26•8 years ago
|
||
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•8 years ago
|
||
(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'.
Reporter | ||
Comment 28•8 years ago
|
||
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•8 years ago
|
||
(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).
Reporter | ||
Comment 30•8 years ago
|
||
(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
Reporter | ||
Comment 31•8 years ago
|
||
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
Reporter | ||
Comment 32•8 years ago
|
||
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*?
Reporter | ||
Comment 33•8 years ago
|
||
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().
Reporter | ||
Comment 34•8 years ago
|
||
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•8 years ago
|
||
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?
Reporter | ||
Comment 36•8 years ago
|
||
(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•8 years ago
|
||
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.
Reporter | ||
Comment 38•8 years ago
|
||
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.
Reporter | ||
Comment 39•8 years ago
|
||
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.
Reporter | ||
Comment 40•8 years ago
|
||
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?
Assignee | ||
Comment 41•8 years ago
|
||
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?
Reporter | ||
Comment 42•8 years ago
|
||
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?
Assignee | ||
Comment 43•8 years ago
|
||
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•8 years ago
|
||
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).
Reporter | ||
Comment 45•8 years ago
|
||
(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•8 years ago
|
||
(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.
Reporter | ||
Comment 47•8 years ago
|
||
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.
Flags: needinfo?(enndeakin)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(enndeakin)
Attachment #8829185 -
Flags: review?(mconley)
Reporter | ||
Comment 48•8 years ago
|
||
Green TB try run with this patch applied here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=74fc0c3a8846bb4a90fed8dccdbd57c0f25847ac
Updated•8 years ago
|
Attachment #8829185 -
Flags: review?(mconley) → review+
Reporter | ||
Comment 49•8 years ago
|
||
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.
Flags: needinfo?(enndeakin)
Reporter | ||
Updated•8 years ago
|
Attachment #8829201 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Component: General → Layout: Form Controls
Product: Thunderbird → Core
Comment 50•8 years ago
|
||
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc004d33b0f9
open start dragging on a menuitem when dropdown is open, r=mconley
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(enndeakin)
Comment 51•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reporter | ||
Comment 52•8 years ago
|
||
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.
Flags: needinfo?(enndeakin)
Attachment #8829185 -
Flags: approval-mozilla-aurora?
Comment 53•8 years ago
|
||
Thanks Jorg for filing the mozmill bug for calendar. I'll file the other one.
Updated•8 years ago
|
status-firefox52:
--- → unaffected
status-firefox53:
--- → affected
Assignee | ||
Comment 54•8 years ago
|
||
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.
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 55•8 years ago
|
||
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
Flags: needinfo?(jcristau)
Comment on attachment 8829185 [details] [diff] [review]
dragonlywhenopen
Fixes a mozmill test, let's uplift to Aurora53
Attachment #8829185 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(jcristau)
Comment 57•8 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•