545838 - Run mozmill tests as part of the build process

Status: RESOLVED FIXED

(Calendar :: Build Config, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I was surprised we don't have a bug for this, I hope I didn't overlook it!

We should have some set of builders that run mozmill tests and turn orange if something fails.

See also bug 405007, where we should have unit test builders. Maybe we can do both of these in one step.

I'd like to see this soon (tm), but at latest for 1.0

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: WiP - v1

This patch is work in progress. The module loading code doesn't work in all cases, especially if a module itself loads certain submodules.

Comment 2

[Merike Sell (:merike)]

(In reply to Philipp Kewisch [:Fallen] from comment #1)
> Created attachment 568488 [details] [diff] [review] [diff] [details] [review]
> WiP - v1

Is it expected result that it breaks the build in current form? I get the following:

make[6]: Entering directory `/home/merike/builds/mail-linux-objdir/calendar/test/mozmill'
/home/merike/builds/mail-linux-objdir/mozilla/config/nsinstall -R -D ../../../mozilla/_tests/mozmill/stage
/home/merike/builds/mail-linux-objdir/mozilla/config/nsinstall -R /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testAlarmDefaultValue.js /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testBasicFunctionality.js /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testLocalICS.js /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testTodayPane.js /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testUTF8.js /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/Makefile.in /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/eventDialog/ /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/mozmilltests.list /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/shared-modules/ /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testAlarmDefaultValue.js /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testBasicFunctionality.js /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testLocalICS.js /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testRecursion/ /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testRecursionRotated/ /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testTodayPane.js /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/testUTF8.js /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/timezoneTests/ /media/Meedia/Mozilla/comm-central/calendar/test/mozmill/views/ ../../../mozilla/_tests/mozmill/stage
/usr/bin/python2.7 /media/Meedia/Mozilla/comm-central/mozilla/config/pythonpath.py -I../../../mozilla/config /media/Meedia/Mozilla/comm-central/mozilla/config/expandlibs_exec.py --uselist --  ccache gcc -o ../../../../mozilla/dist/bin/thunderbird  -Wall -W -Wno-unused -Wpointer-arith -Wcast-align -W -pedantic -Wno-long-long -fno-strict-aliasing -pthread -pipe  -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -fomit-frame-pointer -finline-limit=50    -lpthread    -Wl,-rpath-link,/home/merike/builds/mail-linux-objdir/mozilla/dist/bin:/usr/lib:/usr/local/lib:/lib -Wl,-rpath-link,/usr/local/lib  -Wl,--whole-archive /home/merike/builds/mail-linux-objdir/mozilla/dist/lib/libmozutils.a -Wl,--no-whole-archive -rdynamic -L../../../mozilla/dist/bin -L../../../mozilla/dist/lib  -ldl -lm    
/usr/bin/ld: cannot open output file ../../../../mozilla/dist/bin/thunderbird: No such file or directory
collect2: ld returned 1 exit status
make[6]: *** [../../../../mozilla/dist/bin/thunderbird] Error 1
...

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Nope, not expected. This is likely some trickery regarding the program to execute which differs between mac and linux?

Comment 4

[Merike Sell (:merike)]

Attachment: fix tests

This should fix broken tests not touched by Philipp's patch. I didn't change any of module loading because I was fixing them up using locally installed Mozmill. Those changes need to be added later where needed.

Also, if either of you have any ideas why in testEventDialogModificationPrompt the events remain selected after opening them then I'd be all ears. I suspect it might be some selection logic that has changed in calendar but I'm unable to reproduce it without Mozmill.

Comment 5

[Merike Sell (:merike)]

(In reply to Philipp Kewisch [:Fallen] from comment #3)
> Nope, not expected. This is likely some trickery regarding the program to
> execute which differs between mac and linux?

It seems to fail to create Thunderbird binary altogether. I would have expected it to fail earlier in whatever step should create it though.

Comment 6

[cmtalbert]

I'm not sure how this got by me.  I'm sorry. I'm assuming you're running the 1.5.x version of mozmill.  I will test using that.  Working on it now...

Comment 7

[cmtalbert]

Comment on attachment 570497 [details] [diff] [review]
fix tests

Sorry this took so long to get to.  I reviewed it while in Montreal, forgot to put the changes in bugzilla, and then this week, everything went down :(

These all look good.  So sorry for the delay.

r+

Comment 8

[Merike Sell (:merike)]

(In reply to Clint Talbert ( :ctalbert ) from comment #7)
> So sorry for the delay.

No worries, if it had been blocking something you would've seen pings ;)

Philipp, any updates on the rest of this bug?

Oh, and shouldn't blocking-calendar1.0 flag be removed from bugs that didn't make it?

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Merike (:merike) from comment #8)
> (In reply to Clint Talbert ( :ctalbert ) from comment #7)
> > So sorry for the delay.
> 
> No worries, if it had been blocking something you would've seen pings ;)
> 
> Philipp, any updates on the rest of this bug?
Not yet, I'll see if I can fix the module loading issues.

> 
> Oh, and shouldn't blocking-calendar1.0 flag be removed from bugs that didn't
> make it?
I still need a strategy for those bugs. I don't want to lose the blocking-calendar1.0? bugs and I don't want to be filing bugs every 6 weeks for new tracking flags...

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

This patch gives us initial mozmill support. It includes attachment 570497 [details] [diff] [review] and a few other fixes for the tests.

Note that only 4 tests are actually being run with this patch, but I'd rather have something working that we can build on instead of having this idle until I find time to fix every test.

Actually running mozmill tests requires the mozmill-tests repo being cloned. I'm not sure I've picked the right location (mozilla/testing/mozmill-tests) as I am not quite up to date how mozmill is done for Firefox these days.

Mark, I'm asking you for review only on the build system parts of this patch.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 640084 [details] [diff] [review]
Fix - v1

...and asking Merike for the mozmill parts :)

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Oh right. Note about the module loading issue previously mentioned:

There is no way to require modules from different directories without specifying the full relative path, which could be quite a pain. I've solved this by staging all mozmill files into a directory under the objdir, copying firefox mozmill lib first, then Thunderbird shared modules, then calendar shared modules. I know this might cause a file to be overwritten, but I think this can be handled when the case shows up.

Alternatively I could stage them into separate subdirs, i.e calendar modules into $STAGEDIR/calendar-modules/, thunderbird modules into $STAGEDIR/mail-modules and the firefox mozmill lisb into $STAGEDIR/. If you think this change is worth it, let me know and I'll investigate.

Comment 13

[Merike Sell (:merike)]

Comment on attachment 640084 [details] [diff] [review]
Fix - v1

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

If I understand this splinter thing correctly then I should have managed to leave some comments. It's really awesome to see my tests run and pass after a make call!

::: calendar/test/mozmill/testEventDialog.js
@@ +37,1 @@
>      + 'anon({"anonid":"eventbox"})/{"class":"calendar-event-details"}';

^ a leftover line?

@@ +71,4 @@
>    let startHour = (hour == 23)? hour : (hour + 1) % 24;
> +  let ampm = "";
> +  if (now.toLocaleTimeString().match(/AM|PM/)) {
> +    ampm = (hour > 12 ? " PM" : " AM")

Isn't 12 itself PM?

::: calendar/test/mozmill/shared-modules/calendar-utils.js
@@ +635,5 @@
>    
>    controller.sleep(sleep);
>  }
>  
> +function open_lightning_prefs(aCallback, aParentController, collector, windowTimeout) {

Since it doesn't just open the dialog maybe it should be named handle_lightning_prefs or similar?

@@ +645,5 @@
> +  let timeout = windowTimeout || 30000;
> +  aParentController.window.openOptionsDialog("paneLightning");
> +  aParentController.waitFor(function() {return mozmill.utils.getWindows("Mail:Preferences").length == 1});
> +  let prefsController = new mozmill.controller.MozMillController(mozmill.utils.getWindows("Mail:Preferences")[0]);
> +  prefsController.waitFor(paneLoadedChecker, "Timed out waiting for lightning prefpane to load.");

Shouldn't the above-defined timeout be involved here?

::: calendar/test/mozmill/alarm-defaultvalue.js
@@ +107,5 @@
> +  prefsController.select(new elementslib.ID(prefsController.window.document, "tododefalarmunit"),
> +                         null, null, "days");
> +
> +  // Close the preferences dialog
> +  prefsController.window.close();

This isn't needed as long as it's included in utility function.

::: calendar/test/mozmill/testTodayPane.js
@@ +198,5 @@
>      '/id("messengerWindow")/id("tabmail-container")/id("today-pane-panel")/[1]/id("agenda-panel")/'
>      + '{"flex":"1"}/id("agenda-listbox")/[2]/anon({"class":"agenda-checkbox"})')).getNode();
> +// TODO This is failing, which might actually be an error in our code!
> +//  controller.assertJS(!tomorrow.hasAttribute("checked")
> +//    || tomorrow.getAttribute("checked") != "true");

No idea why it fails but it would be nice to get rid of assertJS altogether as it got removed from Mozmill 2.

::: calendar/test/mozmill/testTaskView.js
@@ +64,5 @@
>                                   return countBefore + 1 == countAfter});
>    
>    // last added task is automatically selected so verify detail window data
> +  controller.assertJS((new elementslib.ID(controller.window.document,
> +    "calendar-task-details-title")).getNode().textContent == title);

A javascript only check would be more future-proof here as well.

Comment 14

[Mark Banner (:standard8)]

(In reply to Philipp Kewisch [:Fallen] from comment #10)
> Actually running mozmill tests requires the mozmill-tests repo being cloned.
> I'm not sure I've picked the right location (mozilla/testing/mozmill-tests)
> as I am not quite up to date how mozmill is done for Firefox these days.

I would go for in-tree mozmill tests like Thunderbird does for various reasons. a) Developers can easily find them and search amongst them via the one location in mxr, b) AFAIK FF isn't actively looking at running MozMill tests in-tree atm, c) importing another repo adds another layer of complexity for developers and for the builders

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 640084 [details] [diff] [review]
Fix - v1

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

The patch looks reasonable, but I'd reconsider pulling in the extra repo, as mentioned previously. It depends what deps you need from there - TB has managed quite well without it.

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I understand your concerns, but I don't really feel well duplicating code just because of a separate repository need. On the other hand, we don't make heavy use of the mozmill-test repo. All that we currently require() is:

* modal dialog (thunderbird mozmill tests have api for this)
* utils
  * getting locale strings (can be replaced with calUtils.jsm)
* prefs
  * getting/resetting preferences (can be replaced with Services.jsm)

IIRC then there was some trouble with module inclusion of the thunderbird modal dialog api, but I will have to revisit this.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Merike (:merike) from comment #13)
> ::: calendar/test/mozmill/testEventDialog.js
> @@ +37,1 @@
> >      + 'anon({"anonid":"eventbox"})/{"class":"calendar-event-details"}';
> 
> ^ a leftover line?
You mean extra spaces? I removed those.

> 
> @@ +71,4 @@
> >    let startHour = (hour == 23)? hour : (hour + 1) % 24;
> > +  let ampm = "";
> > +  if (now.toLocaleTimeString().match(/AM|PM/)) {
> > +    ampm = (hour > 12 ? " PM" : " AM")
> 
> Isn't 12 itself PM?
Indeed, fixed.


> 
> ::: calendar/test/mozmill/shared-modules/calendar-utils.js
> @@ +635,5 @@
> >    
> >    controller.sleep(sleep);
> >  }
> >  
> > +function open_lightning_prefs(aCallback, aParentController, collector, windowTimeout) {
> 
> Since it doesn't just open the dialog maybe it should be named
> handle_lightning_prefs or similar?
Hm well it opens the dialog, waits for the prefpane to be loaded, then lets the caller handle the dialog and closes again. So I wouldn't say its really handling the preferences, since thats whats taken care of in the callback.



> @@ +645,5 @@
> > +  let timeout = windowTimeout || 30000;
> Shouldn't the above-defined timeout be involved here?
Good catch, fixed. 


> ::: calendar/test/mozmill/alarm-defaultvalue.js
> @@ +107,5 @@
> > +  prefsController.select(new elementslib.ID(prefsController.window.document, "tododefalarmunit"),
> > +                         null, null, "days");
> > +
> > +  // Close the preferences dialog
> > +  prefsController.window.close();
> 
> This isn't needed as long as it's included in utility function.
Removed this. I've renamed the function here from setup_.. to handle_prefs_dialog in accordance with your comment above.

> 
> No idea why it fails but it would be nice to get rid of assertJS altogether
> as it got removed from Mozmill 2.
>
> ::: calendar/test/mozmill/testTaskView.js
> @@ +64,5 @@
> >                                   return countBefore + 1 == countAfter});
> >    
> >    // last added task is automatically selected so verify detail window data
> > +  controller.assertJS((new elementslib.ID(controller.window.document,
> > +    "calendar-task-details-title")).getNode().textContent == title);
> 
> A javascript only check would be more future-proof here as well.
Whats the replacement? I was able to use assertJSProperty a few times in that file, but not all instances could be easily replaced.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

Here is v2 of the patch, using only Thunderbird modules (at least for the working tests).

Unfortunately I could only get basic functionality and the today pane test working. In the alarm test, somehow the dropdown for "days" is not selected. It sometimes works when changing the order of the calls aroudn there, but very unreliable. I even had a moment where "48" was entered instead of "50", I wonder where that came from.

For testLocalICS.js there was a problem finding the event box (although its there), so I guess this is just a matter of finding the right path.

Things to remember when fixing the other tests:
* The modal dialog API calls need to be rewritten using Thunderbird' plan/wait_for_modal_dialog functions.
* Thunderbird functions must be imported using collector.getModule()
* I didn't finish fixing all functions in calendar-utils.js to use the tb modal dialog API

Given 2 tests is better than 0 tests, I think we should take this patch as is (modulo review comments) and then fix the remaining tests in follow up.

Comment 19

[Merike Sell (:merike)]

(In reply to Philipp Kewisch [:Fallen] from comment #17)
> (In reply to Merike (:merike) from comment #13)
> > ::: calendar/test/mozmill/testEventDialog.js
> > @@ +37,1 @@
> > >      + 'anon({"anonid":"eventbox"})/{"class":"calendar-event-details"}';
> > 
> > ^ a leftover line?
> You mean extra spaces? I removed those.

No. With the patch applied lines 36-37 read like
    + '[0]/anon({"anonid":"event-container"})/{"class":"calendar-event-selection"}/';
    + 'anon({"anonid":"eventbox"})/{"class":"calendar-event-details"}';
where 36th ends a statement with semicolon and 37th starts appending to nothing. Did you mean to shorten that path or?

Comment 20

[Merike Sell (:merike)]

Comment on attachment 644656 [details] [diff] [review]
Fix - v2

>+  modalDialog.plan_for_modal_dialog("commonDialog",function(attachment){
A space before { would be nice to have.

>+    let input = new elementslib.ID(attachment.window.document, 'loginTextbox');
>+    attachment.waitForElement(input);
>+    input.getNode().value = url;
>+    attachment.click(new elementslib.Lookup(attachment.window.document, '/id("commonDialog")/'
>+      + 'anon({"anonid":"buttons"})/{"dlgtype":"accept"}'));
>+  });

Wouldn't it be safer to have a wait_for_modal_dialog() call here too or is it not needed?

r+ with that and previous comment 19 answered.

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Merike (:merike) from comment #19)
> No. With the patch applied lines 36-37 read like
>     +
> '[0]/anon({"anonid":"event-container"})/{"class":"calendar-event-selection"}/
> ';
>     + 'anon({"anonid":"eventbox"})/{"class":"calendar-event-details"}';
> where 36th ends a statement with semicolon and 37th starts appending to
> nothing. Did you mean to shorten that path or?

Ah now I see what you mean! Good catch, I would have never found that! Fixed.

(In reply to Merike (:merike) from comment #20)
> >+    let input = new elementslib.ID(attachment.window.document, 'loginTextbox');
> >+    attachment.waitForElement(input);
> >+    input.getNode().value = url;
> >+    attachment.click(new elementslib.Lookup(attachment.window.document, '/id("commonDialog")/'
> >+      + 'anon({"anonid":"buttons"})/{"dlgtype":"accept"}'));
> >+  });
> 
> Wouldn't it be safer to have a wait_for_modal_dialog() call here too or is
> it not needed?
Yes, thats probably needed. Done.

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset ee35bef38c14