680203 - Make Lightning tests work with packaged-tests

Status: RESOLVED FIXED

(Calendar :: Build Config, defect, P1)

Description

[Mark Banner (:standard8)]

Currently the only way to run the lightning xpcshell-tests is to cd into objdir/lightning/test and run make xpcshell-tests there.

If an application is building lightning, then it should also run those tests from the global xpcshell-tests command.

Comment 1

[Joshua Cranmer [:jcranmer]]

With my changes to the xpcshell master manifest, calendar tests will be run if Lightning is enabled. However, they are excluded from the packaged test by dint of a neat little trick (packaged tests uses all-test-dirs.list instead of xpcshell.ini; the code eliminates Lightning from the former but not the latter).

Judging from doing local packaging, the part in calendar/lightning which says:
# installing lightning in a thunderbird build causes problems on tinderboxes
# (see bug 406441 and bug 440017), so we need to provide a hook for the
# tinderboxen to disable that.
ifndef DISABLE_LIGHTNING_INSTALL
ifndef MOZ_SUNBIRD
# install Lightning as a global extension in dist/bin/extensions/
INSTALL_EXTENSION_ID = $(XPI_EM_ID)
endif
endif

is no longer necessary to be wrapped in ifndefs.

As lightning is not installed in the packages, we can't run this in packaged tests right now.

Comment 2

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

Attachment: Buildbot changes - v1

These are my plans for buildbotcustom. Adding a few simple lines will extract any extensions packaged with the tests into the extensions directory of the unpacked app.

Mark, can you review this or does someone else need to do so?

Comment 3

[Joshua Cranmer [:jcranmer]]

Bug 979550 may eventually affect this patch.

CC'ing Callek as my main go-to for release engineering related to Thunderbird.

Comment 4

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

One thing you might encounter during review of the previous patch: I unconditionally try to unpack and copy the extensions directory. If there is no extensions/ directory in the tests package, buildbot will currently fail. The unzip part will exit with error code 11, the copy part will exit with 1 and not do the remaining steps.

Doing this in buildbot is cumbersome, I would have to call unzip -l to test if extensions/ is in the archive.

I see two options:
1) Ensure that at least an empty extensions/ directory is added to the archive
2) unzip everything unconditionally

Do you know why option 2 wasn't done from the start?

Comment 5

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

Attachment: comm-central changes - v1

Here are the build system changes. I made one slight change that I haven't run through staging yet, running that overnight to make sure it works.

Comment 6

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

Ok, the extra changes go through with Lightning enabled. It doesn't quite work with Thunderbird without Lightning enabled, because the empty extensions/ directory is not being added to the archive. I can either add a dummy file to the extensions directory, that will unfortunately be copied into the thunderbird/extenions directory, or I need to employ some hacks to get the empty directory added. As this would likely be solved by unpacking everything and making the copy operation conditional, I'll wait for feedback first.

Comment 7

[Mark Banner (:standard8)]

(In reply to Philipp Kewisch [:Fallen] from comment #4)
> I see two options:
> 1) Ensure that at least an empty extensions/ directory is added to the
> archive
> 2) unzip everything unconditionally
> 
> Do you know why option 2 wasn't done from the start?

I believe the reason was to avoid unnecessary extractions - the zip file contains all test files for all the different tests - extracting mochitests & the other ones when you don't need them takes an unnecessary long time when you just want to run the small section of xpcshell tests.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8387836 [details] [diff] [review]
Buildbot changes - v1

You need someone like bhearsum to review this.

I think it does need to work without something in the extensions directory - Firefox doesn't have extensions, so you're likely to break it here.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8387916 [details] [diff] [review]
comm-central changes - v1

Looks reasonable to me. You might want to check with jcranmer if any of this changes with c-c under m-c.

Comment 10

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8387916 [details] [diff] [review]
comm-central changes - v1

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

The changes to testsuite-targets.mk may bitrot one of my patches, but I don't see any significant issues otherwise

Comment 11

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

(In reply to Mark Banner (:standard8) from comment #7)
> (In reply to Philipp Kewisch [:Fallen] from comment #4)
> > 2) unzip everything unconditionally
> > 
> > Do you know why option 2 wasn't done from the start?
> 
> I believe the reason was to avoid unnecessary extractions - the zip file
> contains all test files for all the different tests - extracting mochitests
> & the other ones when you don't need them takes an unnecessary long time
> when you just want to run the small section of xpcshell tests.

mozmill on the other hand extracts everything. I'll see if I can find a good solution without extracting everything and making the code complicated and upload a new patch.

Comment 12

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

Attachment: Buildbot changes - v2

Here is a new patch for the buildbot, ignoring exit code 11 as discussed with bhearsum. I've tested this on staging with a win32 unittest builder, it works both with and without the extensions/ directory present.

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 8390175 [details] [diff] [review]
Buildbot changes - v2

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

This looks okay, but it's pretty risky...can you send me links to your test runs?

Comment 14

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

---- xpcshell ----

Here is an xpcshell test run with Lightning enabled. There may be a test failure in the actual tests, but this isn't caused by the buildbotcustom changes:

http://dev-master1.build.mozilla.org:8951/builders/TB%20Windows%207%2032-bit%20comm-beta%20opt%20test%20xpcshell/builds/18

Here is an xpcshell run without an extensions/ directory. The calendar tests are not run, the other tests succeed: 

http://dev-master1.build.mozilla.org:8951/builders/TB%20Windows%207%2032-bit%20comm-beta%20opt%20test%20xpcshell/builds/13

---- mozmill ----

Here is a mozmill run without an extensions/ directory. The calendar test I added (testBasicFunctionality) is not run, the other tests succeed.

http://dev-master1.build.mozilla.org:8951/builders/TB%20Windows%207%2032-bit%20comm-beta%20opt%20test%20mozmill/builds/8

Here is a mozmill run with Lightning. Two other mozmill tests fail, probably because they don't expect Lightning to be there. Again, this shouldn't affect the buildbotcustom changes and will be taken care of separately:

http://dev-master1.build.mozilla.org:8951/builders/TB%20Windows%207%2032-bit%20comm-beta%20opt%20test%20mozmill/builds/9

Comment 15

[[e-mail] (:bhearsum)]

Comment on attachment 8390175 [details] [diff] [review]
Buildbot changes - v2

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

OK, this looks fine given that output. However, are you sure you need to unpack the entirety of the thunderbird tests zip for the mozmill tests? If you don't, please do it more selectively to safe the I/O and time.

Comment 16

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

I personally don't need to do so for the mozmill tests, it was just this way when I came. I think it would be better to handle that issue in a separate bug. Thanks for the quick review!

Comment 17

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

Comment on attachment 8390175 [details] [diff] [review]
Buildbot changes - v2

https://hg.mozilla.org/build/buildbotcustom/rev/804a86cf6dcc

Comment 18

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

The buildbot changes have gone into production: http://hg.mozilla.org/build/buildbotcustom/rev/606c26367fa3

I'm now taking care of fixing mozmill/xpcshell issues in dependent bugs, then I'll upload an updated patch to enable the tests.

Comment 19

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

Attachment: comm-central changes - v2

All dependent bugs have been filed, patched and are either fixed or in review. Here is an updated patch to enable the tests, I've done some refactoring.

There is one issue I'd love to hear your input on. As both mail and calendar tests are staged into the same directory, there will be a conflict in case there is a test with the same name in both calendar/test/mozmill and mail/test/mozmill. As all Thunderbird tests are prefixed with "test-" this isn't an issue now, but it may be a footgun in the future. If a new test with the same name is added it will cover the existing test, failures in the existing test will go unnoticed.

I initially resolved this by adding -k to the tar extract flags, which caused an error to occur if the files already exist. In this version of the patch I've avoided doing so, because it makes those targets nondeterministic.

A more complex solution would be to add a script a la check-sync-dirs that makes sure the {calendar,mail}/test/mozmill source directories have no files with the same name. Sounds like a good idea, but has a lot of overhead for such a simple issue.

I also investigated staging the files into a calendar-specific subdir. Sounds easy, but involves dynamically modifying include paths in the packaged calendar tests to access the shared modules. Its not trivial to just add a further include path to the module loader, so I dropped this solution.

Let me know what you prefer, I think this patch version is the best compromise despite its weaknesses.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8400037 [details] [diff] [review]
comm-central changes - v2

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

Since all the Thunderbird files are within directories, I don't see this as too much as a problem. Although it might be nice if the calendar files were also vaguely grouped appropriately as well ;-)

Comment 21

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

(In reply to Mark Banner (:standard8) from comment #20)
> Since all the Thunderbird files are within directories, I don't see this as
> too much as a problem. Although it might be nice if the calendar files were
> also vaguely grouped appropriately as well ;-)

I agree, this would be nice. I first wanted to put all tests into a calendar/ subdirectory, but this doesn't work out with shared module loading since the paths would then be different between direct execution (as a developer) and packaged tests.

To solve this, what we could do is stage all mozmill tests into the objdir, this way both Thunderbird and Lightning could copy/link the files into the staged directory and we could put all tests into a common calendar/ subdirectory.

I'll see if I can move the extra files (testBasicFunctionality/testLocalICS) into a subdir at least, this will get things close enough for now, we can handle the staging in a followup if wanted.

For checkin I'll wait until the two dependent patches are checked in and don't fail, then I'll push this one.

Comment 22

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

I'm pushing this now even with bug 998242 not fixed yet, as its mostly a cosmetic change. I will back out the patch tomorrow morning if something goes wrong.

https://hg.mozilla.org/comm-central/rev/f79b49f9149c

Comment 23

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

I've backed this out due to multiple failures of different types. Quite surprising to me, because it has been working on try for quite some time now (aside from the debug build failures which I had skipped).

https://hg.mozilla.org/comm-central/rev/ad94d483149f

Comment 24

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

Giving this one another try now that the tree is open again. Note that I had forgot to backout the lightning-tests.mk file last time, therefore it is not contained in this changeset:

https://hg.mozilla.org/comm-central/rev/db700c5b26b6

Comment 25

[Martin Schröder [:mschroeder]]

Can this bug be closed and marked as fixed? It seems the patch has stuck this time.

Comment 26

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

Yes, thanks for checking!