541235 - mailnews xpcshell tests fail when running as packaged tests

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Robert Kaiser]

For bug 541225, I'm currently running packaged xpcshell tests, listed at http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Ports - but on Mac and Linux, (a number of) the mailnews ones fail.

It seems that we are trying to write to a mailnews/ subdirectory in _tests/ but that fails on those OSes. Apparently, the more liberal rules for writing on the file system allows us to create and use that dir on Windows, and the tests succeed there (the failure we are still seeing is a different bug that also happens on normal test runs right now).

I would be happy if we could change this in a way that we write to a directory that always succeeds.

Comment 1

[Robert Kaiser]

I talked to Standard8 on IRC about this, he seems to have at least some clue about this.

Comment 2

[Serge Gautherie (:sgautherie)]

Fwiw, tests are supposed to use the test profile directory, I think.

Comment 3

[Serge Gautherie (:sgautherie)]

Ftr, the failure number is slowly increasing:
ftb, I'm not checking them and just assume they are new mailnews tests being added...

Comment 4

[Mark Banner (:standard8)]

The reason the tests fail is:

- mailDirService.js assumes that the process directory is <objdir>/mozilla/dist/bin. This is reasonable for situations where the build is part of an in-tree process.

- It then tries and creates a "profile" directory in "../../_tests" called "mailtest".

When running tests in the packaged mode this can fail because "../.." is outside the test directory and sometimes outside the file system.


When we created mailDirService.js we did so with roughly the following requirements:

1) The directory must be in a consistent location so that developers know where to find it.

I chose <objdir>/mozilla/_tests/mailtest for the location as a) it wasn't dumping yet more junk in <objdir>/mozilla/dist/bin, b) _tests seemed a sensible place to put it.

2) At the end of the test the profile directory must be left in-place and untouched. This is to aid debugging with check-one/check-interactive - having the profile available for mailnews testing is sometimes very useful when trying to debug tests etc.

3) As a result of 2) a test which uses the profile directory must delete it before the test starts.


We could switch to using the profile location code provided by xpcshell-tests.

This was implemented recently. There are several issues with this code.

- It uses a random folder name (based in the temporary directory).
- It deletes the profile directory after the test runs.

These both contravene requirements 1 and 2 above. check-interactive can be used to hold the test in a state where the profile directory isn't deleted, but that's a PITA if you're just needing to run the test and see the results of the profile and not debugging the bits of code as well.


One possible reasonable solution would be to change the core xpcshell-profile code so that it uses a consistent directory name in the temp directory (or at least a consistent basename with CreateUnique if necessary), and it implements 2 and 3 above. I'm not sure if they will object to this.

The only other way would be to always just use a sub-directory where the process was run from. Not sure if there would be any other issues with this.

Comment 5

[(not currently active) Ted Mielczarek]

I'd definitely suggest using the existing do_get_profile code. If you need additional features, we can figure out sensible ways to make them work. For example, we could make "check-one / check-interactive" direct runxpcshelltests.py to put the profile in a known location and not delete it after running the test.

Comment 6

[Kent James (:rkent)]

I was an early proponent of not deleting the profile directory after running a test, and I still think that is important. If you run in a test-driven development mode, as I frequently do, it is really important to be able to examine the contents of the files that you are creating. I would hate to see us go backwards on this, making test development more difficult, when we are trying to tighten up on testing requirements.

Comment 7

[Robert Kaiser]

When tests run automatically on our test boxes, it's reasonable that everything gets deleted. When running locally for development, it's reasonable to leave things behind unless it's just a simple sanity check run. Maybe an option to runxpcshelltests.py to not delete it (or also making this triggered by an env var so you can easily pass it to a call via the build system) would be a good idea - but mailnews should use the general mechanism for where to put files, and we should strive to improve the general mechanism to achieve what we want.

Comment 8

[Robert Kaiser]

Attachment: just use the profile directory

This patch just switches mailDirService.js to use do_get_profile() which should fix packaged tests from all we know.
If mailnews people want the platform mechanism to be extended to have a flag to keep the profile directory around, I think this should go into yet another related bug - and as ted said, the guys over there are open to such a thing.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> If mailnews people want the platform mechanism to be extended to have a flag to
> keep the profile directory around, I think this should go into yet another
> related bug - and as ted said, the guys over there are open to such a thing.

Agreed that should be a separate bug, so I've filed bug 550860. However I also don't want to loose this capability (as for instance, I was just using it earlier today), so I think we should be fixing bug 550860 before this one.

I may have time to work on that bug before the end of the week, but I can't guarantee it, so anyone should feel free to pick it up if they have time.

Comment 10

[Serge Gautherie (:sgautherie)]

A solution for this is badly needed:
getting a timeout instead of the pass/fail numbers makes it rather painful to track the new failures and "impossible" to track the intermittent ones.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)

On Windows...

Comment 12

[Serge Gautherie (:sgautherie)]

The situation got even worse today (after bug 556352 landing, I assume):
it looks like the tests are now sorted,
while I would rather like that,
it means that other failures are now buried in the middle of the "180" mailnews failures, making the formers very painful to find.

As there has been no progress in bug 550860, could we PLEASE fix this bug with KaiRo's patch asap?
(That's more than 2 months that we're bearing with this...)

Comment 13

[Serge Gautherie (:sgautherie)]

Eventually, bug 559681 was a closely related issue:
why not just add 'mailtest/' to 'xpcshell/mailnews/' instead of to temp or profile directory?
This should fix both this bug and bug 550860 at once, shouldn't it?

Comment 14

[Robert Kaiser]

(In reply to comment #13)
> why not just add 'mailtest/' to 'xpcshell/mailnews/' instead of to temp or
> profile directory?

The profile directory is the right thing to use, see comment #5.
Bug 550860 is about finding a general solution that also allows better debugging of _any_ xpcshell test that uses profile data. That's also the right thing to do and I thought you as someone frequently investigating tests would cheer for that! ;-)

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)

> The profile directory is the right thing to use, see comment #5.

I (would) agree.

> Bug 550860 is about finding a general solution

Good point, I overlooked this in my comment.

> I thought you [...] would cheer for that! ;-)

I would, but it's been 3 months with no (reviewed) solution :-(

Let me put it another way then:
could mailDirService.js put 'mailtest/' in 'xpcshell/mailnews/', as a workaround to fix the _immediate_ issue SeaMonkey suffers from (= unbreak packaged tests)?
P-L-E-A-S-E, Mark!

NB: Then you can debate and wait as long as you want to fix all this the righter way.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 428922 [details] [diff] [review]
just use the profile directory

r=Standard8 for landing after bug 550860 lands in mozilla-central (which has just been reviewed).

Please also post to mda.thunderbird & mda.seamonkey about the change in behaviour.

Comment 17

[Justin Wood (:Callek)]

Pushed as: http://hg.mozilla.org/comm-central/rev/f33643d4a469

If this corrects many of our mailnews/* tests I'll resolve and use followups for individual issues that appear.

Comment 18

[Justin Wood (:Callek)]

only one failure for the first run on the seamonkey side. Safe to say this fixed it (going to wait almost 24 hours before *I* file the bug on the exposed failure so that I can get proper data if its random, perma, other platforms, etc.)