The mozmill recurrence tests for calendar are failing at the moment. Most of it seems to be because of modules missing in the standard comm-central repo.

This ist what I've done so far. Main things are: Use window-helpers module functions for modal dialogs ISO modal-dialog module which is not available by standard. To enable this in the calendar-utils module, I changed it to a more "mozmill-standard" state, renaming it to "test-calendar-utils.js" The most prominent change is the remodeling of occurrence-prompt handling: +function handleOccurrencePrompt(controller, element, mode, selectParent, attendees) { + modalDialog.plan_for_modal_dialog("Calendar:OccurrencePrompt", + function(dialog) { + if(attendees) { + acceptSendingNotificationMail(); + } + if (selectParent == true) { + dialog.waitThenClick(new elementslib.ID(dialog.window.document, + "accept-parent-button")); + } else { + dialog.waitThenClick(new elementslib.ID(dialog.window.document, + "accept-occurrence-button")); + } + } + ); + if (mode == "delete") { + controller.keypress(element, "VK_DELETE", {}); + } else if (mode == "modify") { + controller.doubleClick(element); + } + modalDialog.wait_for_modal_dialog("Calendar:OccurrencePrompt", 30000); +} The main change ist that the element which has to be clicked on is passed into the function. This is needed, because the opening of the dialog has to happen between "plan_for_modal_dialog" and "wait_for_modal_dialog". To re-use as much code as possible, the logic to determine which button to choose is put into the function. The next thing to do would be to get rid of the "utils.js" module from the mozmill-tests repo, which would itself require additional modules from there. As far as I see it now, it is used only for string-fetching. My current Idea is to prrovide a function for this in the calendar-utils module.

Comment on attachment 8387054 [details] [diff] [review] WIP patch 0.1 Review of attachment 8387054 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. So we need to remove refs to the mozmill-tests repo? I guess its not being made available? ::: calendar/test/mozmill/recurrence/testBiweeklyRecurrence.js @@ +7,5 @@ > > const sleep = 500; > var calendar = "Mozmill"; > var hour = 8; > +var calUtils; calUtils could be confused with calUtils.jsm. I suggest calTestUtils, or just caltest. Or maybe "cu" in spirit of what thunderbird does? ::: calendar/test/mozmill/recurrence/testLastDayOfMonthRecurrence.js @@ +39,5 @@ > event.waitForElement(new elementslib.ID(event.window.document, "item-repeat")); > event.select(new elementslib.ID(event.window.document, "item-repeat"), undefined, undefined, > "custom"); > + wait_for_modal_dialog("Calendar:EventDialog:Recurrence", > + 30000); lets not hardcode if we don't need to. Isn't there a global for this? I see "sleep" in the next chunk.

(In reply to Philipp Kewisch [:Fallen] from comment #2) > Looks good. So we need to remove refs to the mozmill-tests repo? I guess its > not being made available? It has been like this for over a year now, so I think these files won't be available to us soon. Maybe Merike can give more information on this. > > +var calUtils; > > calUtils could be confused with calUtils.jsm. I suggest calTestUtils, or > just caltest. Or maybe "cu" in spirit of what thunderbird does? good point. > ::: calendar/test/mozmill/recurrence/testLastDayOfMonthRecurrence.js > @@ +39,5 @@ > > event.waitForElement(new elementslib.ID(event.window.document, "item-repeat")); > > event.select(new elementslib.ID(event.window.document, "item-repeat"), undefined, undefined, > > "custom"); > > + wait_for_modal_dialog("Calendar:EventDialog:Recurrence", > > + 30000); > > lets not hardcode if we don't need to. Isn't there a global for this? I see > "sleep" in the next chunk. sleep is a much shorter period of 500ms here which is used for short waits for controls to reach a state. It is too short for a timeout of a dialog in my opinion. but I can well put the timeout in a var or const. If Merike has no objections, I proceed this way with this Patch.

Comment on attachment 8387054 [details] [diff] [review] WIP patch 0.1 Review of attachment 8387054 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. (In reply to Philipp Kewisch [:Fallen] from comment #2) > Looks good. So we need to remove refs to the mozmill-tests repo? I guess its > not being made available? I think modal dialog api is still available from http://hg.mozilla.org/qa/mozmill-tests/file/tip/firefox/lib/modal-dialog.js There might have been changes to it meanwhile though. As it is used only for Firefox tests they are probably not putting any extra attention to make sure all changes work equally well on Thunderbird too should there be minor differences somewhere. For that reason I think it makes more sense to use what Thunderbird uses for its tests. ::: calendar/test/mozmill/recurrence/testLastDayOfMonthRecurrence.js @@ +39,5 @@ > event.waitForElement(new elementslib.ID(event.window.document, "item-repeat")); > event.select(new elementslib.ID(event.window.document, "item-repeat"), undefined, undefined, > "custom"); > + wait_for_modal_dialog("Calendar:EventDialog:Recurrence", > + 30000); It's probably a good idea to put it into calendar utils module as a constant. Usage of sleeps is not exactly consistent right now, better to not make it any worse by adding magic numbers. ::: calendar/test/mozmill/shared-modules/test-calendar-utils.js @@ +58,3 @@ > * @param controller - Mozmill window controller > + * @param element - Mozmill element which will open the dialog > + * @param mode - string to determine which action to exec on element. This should have examples so that you don't need to read the function body to know what to pass as mode. @@ +662,5 @@ > +// exports.handleAddingAttachment = handleAddingAttachment; > +// exports.handleOccurrencePrompt = handleOccurrencePrompt; > +// exports.setData = setData; > +// exports.switchToView = switchToView; > +// exports.open_lightning_prefs = open_lightning_prefs; Interesting, I think the way Mozmill handles modules has changed over time so that exports are not needed any more.

(In reply to Merike (:merike) from comment #4) > Comment on attachment 8387054 [details] [diff] [review] > @@ +662,5 @@ > > +// exports.handleAddingAttachment = handleAddingAttachment; > > +// exports.handleOccurrencePrompt = handleOccurrencePrompt; > > +// exports.setData = setData; > > +// exports.switchToView = switchToView; > > +// exports.open_lightning_prefs = open_lightning_prefs; > > Interesting, I think the way Mozmill handles modules has changed over time > so that exports are not needed any more. Currently Mozmill does this with the modules whose names start with "test..." one of the reasons I renamed the file. With the rest of the comments, I will continue the work.

The patch now fixes all tests in the "recurrence" folder. It does a lot of changes to the calendar-utils shared-module. Because of this fact it has potential to break some of the other tests. But since they are nearly all not working at the moment and I plan to fix them anyway, I think we can live with that. > calUtils could be confused with calUtils.jsm. I suggest calTestUtils, or > just caltest. Or maybe "cu" in spirit of what thunderbird does? since I added an installInto() function to the utils, this variable is only used twice in setupModule(). Thus I changed it to cu as the TB-Tests do. > lets not hardcode if we don't need to. Isn't there a global for this? I see > "sleep" in the next chunk. I changed these values to constants named SLEEP, SHORT_SLEEP, and TIMEOUT. These are set in the calendar-utils module and installed into the tests via installInto() to be used there also. Since by removing calUtils.[member] calls throughout the tests nearly touches all lines anyway, I also cleaned up indentation and whitespace. This is quite a large pytch by now, but it makes all recurrence tests pass.

caught some small style nits again...

Comment on attachment 8391752 [details] [diff] [review] Patch V1.1 Review of attachment 8391752 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just a few nits. I haven't actually tested it though. ::: calendar/test/mozmill/recurrence/testBiweeklyRecurrence.js @@ +38,4 @@ > // check day view > for(let i = 0; i < 4; i++){ > controller.assertNode(new elementslib.Lookup(controller.window.document, box)); > + forward(controller, 14); Maybe we could give the forward function a more meaningful name? ::: calendar/test/mozmill/shared-modules/test-calendar-utils.js @@ +106,5 @@ > + function(dialog) { > + if(attendees) { > + acceptSendingNotificationMail(); > + } > + if (selectParent == true) { drop the " == true" part.

Comment on attachment 8391752 [details] [diff] [review] Patch V1.1 Review of attachment 8391752 [details] [diff] [review]: ----------------------------------------------------------------- Generally looks good to me. A few notes and questions though. Do the three currently enabled tests work for you with this change? I see "Test Failure: Module "./shared-modules/calendar-utils" not found" for these. Do all recurrence test work for you? I'm getting 3 failures: SUMMARY-UNEXPECTED-FAIL | testWeeklyUntilRecurrence.js | testWeeklyUntilRecurrence.js::testWeeklyUntilRecurrence EXCEPTION: No item selected for element ID: item-repeat at: controller.js line 869 MozMillController.prototype.select controller.js 869 testWeeklyUntilRecurrence testWeeklyUntilRecurrence.js 35 SUMMARY-UNEXPECTED-FAIL | testWeeklyNRecurrence.js | testWeeklyNRecurrence.js::testWeeklyNRecurrence EXCEPTION: Expression "{"flex":"1"}" returned null. Anonymous == false at: elementslib.js line 486 Lookup.prototype.getNode/reduceLookup elementslib.js 486 Lookup.prototype.getNode elementslib.js 501 MozMillController.prototype.assertNode controller.js 912 testWeeklyNRecurrence testWeeklyNRecurrence.js 44 SUMMARY-UNEXPECTED-FAIL | testDailyRecurrence.js | testDailyRecurrence.js::testDailyRecurrence EXCEPTION: Timeout exceeded for waitForElement Lookup: /id("messengerWindow")/id("tabmail-container")/id("tabmail")/id("tabpanelcontainer")/id("calendarTabPanel")/id("calendarContent")/id("calendarDisplayDeck")/id("calendar-view-box")/id("view-deck")/id("day-view")/anon({"anonid":"mainbox"})/anon({"anonid":"scrollbox"})/anon({"anonid":"daybox"})/[0]/anon({"anonid":"boxstack"})/anon({"anonid":"topbox"})/{"flex":"1"}/{"flex":"1"}/{"flex":"1"}/{"tooltip":"itemTooltip","calendar":"mozmill"} at: utils.js line 447 TimeoutError utils.js 447 waitFor utils.js 485 MozMillController.prototype.waitFor controller.js 685 MozMillController.prototype.waitForElement controller.js 701 testDailyRecurrence testDailyRecurrence.js 39 I'll try updating comm-central to the latest if you're not seeing this. ::: calendar/test/mozmill/recurrence/testWeeklyNRecurrence.js @@ +96,4 @@ > let days = '/id("calendar-event-dialog-recurrence")/id("recurrence-pattern-groupbox")/' > + + 'id("recurrence-pattern-grid")/id("recurrence-pattern-rows")/id("recurrence-pattern-period-row")/' > + + 'id("period-deck")/id("period-deck-weekly-box")/[1]/id("daypicker-weekday")/anon({"anonid":"mainbox"})/'; > + Shouldn't it be aligned differently here? Similar to how it's done for "let input ..." below? ::: calendar/test/mozmill/shared-modules/test-calendar-utils.js @@ +201,4 @@ > // pick year > controller.click(new elementslib.Lookup(controller.window.document, miniMonth > + 'anon({"anonid":"minimonth-header"})/anon({"anonid":"yearcell"})')); > controller.sleep(500); Shouldn't this 500 get replaced as well?

(In reply to Merike (:merike) from comment #9) > Do the three currently enabled tests work for you with this change? I see > "Test Failure: Module "./shared-modules/calendar-utils" not found" for these. I overlooked these --> Fixed. > Do all recurrence test work for you? I'm getting 3 failures: I did not have these failures on windows, but could reproduce them on my linux-VM They were related to controls/elements not being ready. I added a sleep() and some waitfor() and now all tests run through there as well. The two style nits are also fixed as well as the comments from philipp Since Comm-Central is still closed, can be marked for checkin if review is +

Forgot the last qref. Sorry.

Comment on attachment 8402086 [details] [diff] [review] Patch V1.3 Review of attachment 8402086 [details] [diff] [review]: ----------------------------------------------------------------- I've just briefly skimmed the code and it looks fine. Could you do a tryserver run with this patch (and patch in review on bug 680203) before you set checkin-needed? Here is the trychooser syntax: try: -b o -e -p linux,linux64,macosx64,win32 -u mozmill -t none

Not sure what the status here was because I didn't read through all comments, but I started a try run anyway. Possibly you were waiting for the module changes in your other bug? https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=c0b77aef07db Two little nits I found while refreshing: b/calendar/test/mozmill/shared-modules/test-calendar-utils.js:70: Redundant blank line b/calendar/test/mozmill/shared-modules/test-calendar-utils.js:108: Missing blank before `(` + if(attendees) {

The status is, that I am still waiting for the MAC-Tests to pass for which I don't have the possibility of testing right now. I have a patch, that makes all other platforms pass. The two nits will be addressed.

This patch makes the tests pass on MAC finally. not tested again on the other platforms yet, because the builds are not working at the moment. This patch has to be applied on top of the one in bug 1020574. The notes on the patch there also apply to this one.

Comment on attachment 8443935 [details] [diff] [review] Patch V1.4 Review of attachment 8443935 [details] [diff] [review]: ----------------------------------------------------------------- I've briefly skimmed the code and it looks fine. Here is a try build including your patches, lets hope they pass. Go ahead and push once you have a green try run. https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=562436af1f0f

can anyone maybe give me a hint? I have the tests running fine on 2 linux VMs at home. But on try, the tess always fail when setting a weekly recurrence via the "custom" dialog. The assert checked for the first checkbox fails: https://tbpl.mozilla.org/?tree=Thunderbird-Try&pusher=Mozilla@Adrario.de I can't reproduce this at home and have no idea what goes wrong. I made sure everything is loaded before checking, I tried setting the timeout up to 30 sec. I'm puzzled. the latest patches are to be found in http://hg.mozilla.org/users/Mozilla_Adrario.de/mq-patches/ The first 4 patches need to be applied and TB rebuilt with the first one applied.

Hey Markus, sorry for not getting back to you earlier. The try run has fallen off the end, can you do another one and paste the relevant parts of the error message here? Can you also verify it is actually this patch causing it, and not some intermittent error?

This is the new tryrun: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=26d7a288c862 Since the runs before that with other patches show passed tests, I'm pretty sure the recurrence tests cause this.

If I'm reading that failure right then it seems to time out when trying to select "custom" in the event dialog. There's a call to menulistSelect defined in calendar-utils that seems to call waitFor without specifying timeout. Maybe you need to apply custom timeout there instead? I also don't get this failure locally but that's what I'd try next :)

Yeah, that's right. The default timeout is 5 or 30s (documentation is not quite clear about this. I tried setting the timeout manually up to 60s and the failure remains. :-(

What is this Mac bug you're working around by using custom select function for menulist? If Mozmill's select fails on mac and works on linux then you probably need to go with small steps from original select to the new one and see when it starts to fail on try. Mozmill does not just call click() twice it sleeps between them and actually fakes mouse actions with EventUtils.synthesizeMouse: http://hg.mozilla.org/comm-central/file/ad8d68fdeee3/mail/test/resources/mozmill/mozmill/extension/resource/modules/controller.js#l848 Some of that must be crucial somehow.

Right now Mozmill tests are not running on tryserver. As soon as they are fixed, I will try a run with the original mozmill select() function to see what MacOS does with this. maybe we'll end up with a if(MacOS) check...

I switched the menulist-select function back to the built in select function of Mozmill and the outcome is the same: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b308beefca71 So the problem must be elsewhere. If only I could reproduce the behaviour locally...

Markus, do you you plan to work on this in the near future? If not I might be able to spend some time on this next week or the week after.

Hi Merike, I am quite busy at the moment and since I am also out of ideas on this one, feel free to have a try. I'll see, that I have my MQ-repo updated to the latest version and mail you the link.

OK, patches are up to date. Link to repo is in comment 17. at least first 3 patches in series are needed.

Waiting a year and together with the changes of Philipp and me in the shared-modules, everything seems to work now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c20670de86e3a50d5f9948ddf7925328d7dde891 On Mac I also had a clean test-run yesterday: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e472d537301effa7b18cca189e97657e8dfb3cf4

Comment on attachment 8803695 [details] [diff] [review] Patch V2.0 As I noticed when doing a try push for the patch from bug 1303625, this one doesn't apply on top of that. I got several failures. Can you please update this patch once the other one landed?

debitroted patch.

Comment on attachment 8819672 [details] [diff] [review] Patch V2.1 Review of attachment 8819672 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, I also did a successfull try push: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5f536092527513923a9ee18c4dcd544e6112e0d0&selectedJob=26410 r+ with comments considered. ::: calendar/test/mozmill/mozmilltests.list @@ +2,4 @@ > testBasicFunctionality.js > testTodayPane.js > testLocalICS.js > +recurrence Can you add a prefix to our folder names for mozmill tests? We share the namespace with other cc components, so a prefix that makes it obvious that this folder contains calendar tests might be helpful (we should use such then for our other folders, too). ::: calendar/test/mozmill/recurrence/testWeeklyNRecurrence.js @@ +130,5 @@ > + // starting from Monday so it should be checked. We have to wait a little, > + // because the checkedstate is set in background by JS. > + recurrence.waitFor(() => { > + return recurrence.assertChecked(reclookup(`${REC_DLG_DAYS}/{"label":"${mon}"}`)); > + }, 30000); Do we really need 30000 here?

I have to ask for review again for this one. I found a new way of sensing, when the view has finished loading. For this a introduced the function ensureViewLoaded in test-calendar-utils. Also I moved additional Paths to the calendar-utils and made use of the repaired "setData" Function. Sorry for the additional review-work.

Try-Run is here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1da3df05747f9c2ce75dc45a73c8b6a4491e4aaa

Comment on attachment 8822660 [details] [diff] [review] Patch V2.2 Review of attachment 8822660 [details] [diff] [review]: ----------------------------------------------------------------- No problem, you're welcome. Looks good apart from some style nits. r+ with those fixed. ::: calendar/test/mozmill/mozmilltests.list @@ +1,4 @@ > testAlarmDefaultValue.js > testBasicFunctionality.js > testLocalICS.js > +recurrence You didn't fix the folder name as requested (to e.g. calRecurrence); also move the folder to the top of the list then. ::: calendar/test/mozmill/recurrence/testBiweeklyRecurrence.js @@ +27,5 @@ > viewForward, > + deleteCalendars, > + createCalendar, > + menulistSelect > + } = collector.getModule("calendar-utils")); reduce indentation before the curly bracket. ::: calendar/test/mozmill/recurrence/testLastDayOfMonthRecurrence.js @@ +36,5 @@ > collector.getModule("calendar-utils").setupModule(); > Object.assign(module, helpersForController(controller)); > > + ({ plan_for_modal_dialog, wait_for_modal_dialog } = > + collector.getModule("window-helpers")); Put the closing parenthesis to the next line. @@ +58,5 @@ > > event.click(eventid("button-saveandclose")); > }); > > // year mo day correct row in month view make this // data tuple: [year, month, day, row in month view] // note: month starts here with 1 for January ::: calendar/test/mozmill/recurrence/testWeeklyNRecurrence.js @@ +39,5 @@ > collector.getModule("calendar-utils").setupModule(); > Object.assign(module, helpersForController(controller)); > > + ({ plan_for_modal_dialog, wait_for_modal_dialog } = > + collector.getModule("window-helpers")); Again: closing parehntesis to new line. ::: calendar/test/mozmill/shared-modules/test-calendar-utils.js @@ +26,5 @@ > +var EVENTPATH = `/{"tooltip":"itemTooltip","calendar":"${CALENDARNAME.toLowerCase()}"}`; > +var REC_DLG_ACCEPT = ` > + /id("calendar-event-dialog-recurrence") > + /anon({"anonid":"buttons"})/{"dlgtype":"accept"} > + `; Fix the indentation here and below: var REC_DLG_ACCEPT = ` /id("calendar-event-dialog-recurrence") /anon({"anonid":"buttons"})/{"dlgtype":"accept"} `; @@ +116,5 @@ > + */ > +function ensureViewLoaded(controller) { > + let { sleep } = helpersForController(controller); > + controller.waitFor(() => > + controller.window.getViewDeck().selectedPanel.mPendingRefreshJobs.size == 0); put the closing parenthesis to the new line: controller.waitFor(() => controller.window.getViewDeck().selectedPanel.mPendingRefreshJobs.size == 0 );

(In reply to [:MakeMyDay] from comment #34) > You didn't fix the folder name as requested (to e.g. calRecurrence); also > move the folder to the top of the list then. Just a quick drive-by, I'd prefer cal-recurrence for the folder name.

cal-recurrence it will be then. I had a test-failure in the test-Run before checkin on linux-32 debug [1]. It seems, that the handling of the recurrence dialog sometimes fails. I thought, that I got rid of these failures. I tried to harden the setRecurrence function a little more and now got one successful test-runs [2]. Today there is another failure [3] Shall we disable this test for now, or wait with all of them? [1]: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b9fc6e66f1b7dbb6751b461499118431efe7554c&selectedJob=65573522&filter-searchStr=Linux%20debug%20Mozmill%20(Z) [2]: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fadbb599ac2357bf0f301e874aa5c29917d8d142 [3]: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fb871d138ff7eba7a2ac515b0e939264c4952493&selectedJob=65672431&filter-searchStr=Linux%20x64%20debug%20Mozmill%20(Z)

But now the failure in testLastDayOfMonthRecurrence.js shows up in Linux64 debug in your today's try push As this is intermittent, it's probably sufficient to increase the timeout. Let's give it another try with that. Regarding the today pane test, can you push some debug code to try to analyse the output of both of the compared arguments in the respective line 63: controller.assertJS(lookup(dayPath).getNode().mDate.icalString == getIsoDate()); We have first day in a month failures also in other tests with icla.js only (which is also used for mozmill tests), so maybe this is a bug in ical.js and today would be an opportunity to debug this.

With the sleep set to 500ms, now 3 try-runs were without failures. Shall I go ahead and push then?

Sorry for the delay. That sounds solid enough to give it a shot, so yes, go ahead.

I put in additional waitFors at two points after view changes. Lets see... pushed to comm-central -> https://hg.mozilla.org/comm-central/rev/eab361faf708c751aa61a8245d9bea83733a8e6e

Unfortunately, there are still two test failures (Linux32 only). Are that the same you meant to fix with adding the addional waitFor's? https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=eab361faf708c751aa61a8245d9bea83733a8e6e

Unfortunately not. These two failures are the first tests after closure of the event dialogue. So there seems to be an error in saving the event but I have no idea why. I keep investigating. Can you link a bug with the intermittent failure to this?

Unfortunately I can't atm, which reminds me to request a reset of my ldap password (it's really not handy that there's other than for MoCo staff no self service for that for the community). But probably someone else in #maildev can.