1585162 - Convert calendar Mozmill tests to Mochitest

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, task)

Description

[Geoff Lankow (:darktrojan)]

Next part of the plan to take over the world remove Mozmill.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1585162-calendar-mozmill-mochitest-1.diff

On the whole this patch is quite boring, but there are some interesting bits at the beginning and end.

The general idea here is for the tests to do exactly what they did before, but under the Mochitest framework instead of the Mozmill one. That presents a few small problems, Mochitest expects you to actually check things, but Mozmill just wants to make sure everything happened and you get to the end of the test. Hence the checks that true is truthy I inserted at the end of a lot of tests.

Later on I'll try to get rid of the notion of a controller and rewrite bits to be more Mochitest. For now I'm using calendar as a bit of a proof of concept before tackling the other tests, and also so I can get rid of the weird build system hacks that were invented to make it work.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9097498 [details] [diff] [review]
1585162-calendar-mozmill-mochitest-1.diff

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

::: calendar/test/browser/eventDialog/browser_alarmDialog.js
@@ +80,2 @@
>  
> +  Assert.ok(true);

maybe you want to add a comment to these, like 
Assert.ok(true); // Make sure we got here!

Comment 3

[Paul Morris [:pmorris]]

Comment on attachment 9097498 [details] [diff] [review]
1585162-calendar-mozmill-mochitest-1.diff

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

I like the suggestion Magnus made.  r+ with those comments added.  I ran the tests with the patch applied and they succeeded:
```
mochitest-browser
~~~~~~~~~~~~~~~~~
Ran 276 checks (242 subtests, 34 tests)
Expected results: 276
Unexpected results: 0
OK
```

::: calendar/test/browser/browser_localICS.js
@@ +27,5 @@
>  
> +// Unique name needed as deleting a calendar only unsubscribes from it and
> +// if same file were used on next testrun then previously created event
> +// would show up.
> +var calendarName = new Date().getTime() + "";

Probably out of scope here since this is existing code, but I prefer this more explicit way of converting to a string:
String(new Date().getTime())

Comment 4

[Paul Morris [:pmorris]]

Meant to say: this is very cool! It will be great to drop the legacy mozmill dependency.

Comment 5

[Jorg K (CEST = GMT+2)]

Isn't there an issue though? Mochitests only run on opt builds because, IIRC, the leak analysis goes mad on debug builds. So what are we going to do about this?

Comment 6

[Geoff Lankow (:darktrojan)]

Yes there is. I'm aware and working on it. That's another reason I'm only modifying the calendar tests for now.

Comment 7

[Jorg K (CEST = GMT+2)]

I mentioned this on IRC: Can we please not lump the Calender tests into M(bct) on the treeherder. This would be a sheriffing nightmare. I also find "bct", apparently browser chrome thunderbird, not particularly helpful. It would be much better to name them after functional groups, like ext for the current extension tests(?), cal for calendar, and then one abbreviation for one or a group of previous MozMill tests:
https://searchfox.org/comm-central/source/mail/test/mozmill
Something like acc, ab, att, cld, cmp, cp, etc. That might be a bit more effort, but will help lots of people a lot of time. Say someone works on an address book bug, with some likelihood they would cause failures in ab, cmp but not acc. Having a single thing go red is really unhelpful.

Comment 8

[Geoff Lankow (:darktrojan)]

It will be no worse than before except the logs will give you the actual full path to the test. Plus, all the calendar tests will be in the same job (I suppose theoretically they could be in two neighbouring jobs once we have more) instead of scattered across all of them, so that's helpful.

In any case I don't think this is the bug to be talking about such things.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2a2928f94f6b
Convert calendar Mozmill tests to Mochitest; r=pmorris

Comment 10

[Jorg K (CEST = GMT+2)]

Looks like running test locally on Windows is broken. Geoff suggested to change mail/test/resources/mozmill/mozmill/extension/bootstrap.js like so:

-  modulesFile.initWithPath(env.get("TESTING_MODULES_DIR"));
+  modulesFile.initWithPath(env.get("TESTING_MODULES_DIR").replace(/\//g, "\\"));

That makes it work for the one test I tried ;-)

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: 1585162-windows-paths.diff

I really do hate Windows.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 9098487 [details] [diff] [review]
1585162-windows-paths.diff

Thanks. With all humour, I don't think the commit message is quite appropriate :/

Comment 13

[Jorg K (CEST = GMT+2)]

I wonder whether this will tell us anything:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8468093840e3411d124001dc72f30bcc1a21b8f0

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/de5d9afa75a3
follow-up: Fix MozMill initialisation for running tests locally on Windows. r=jorgk

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/926c2b57bd01
follow-up - Attempt to improve reliability of some tests; rs=bustage-fix