887404 - Browserglue tests should be able to run in parallel

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Mihnea Dobrescu-Balaur (:mihneadb)]

Right now running the tests in browser/components/places/tests/unit with the patch from bug 887054 does not work, tests are failing.

Comment 1

[Marco Bonardo [:mak]]

taking for investigation, though if you find a solution earlier feel free to steal

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: fix

The problem was the following:

test_browserGlue_distribution.js creates a distribution.ini file in the bin directory to check this functionality. The distribution.ini file *has* to live in the bin directory, and that directory is common to all processes.

This distribution.ini file is loaded by all the processes and it changes the bookmarks for all of them.

From what I looked and asked around, there is no way to disable this functionality or to have the distribution.ini file live somewhere else.

This is why I'm disabling this test (for now at least). I asked about maybe having a pref to disable this behaviour and I was told that is not desired.

Comment 3

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 769200 [details] [diff] [review]
fix

Turns out tests are still failing. Also, no need to disable browserGlue_distribution, we can just mark it run-sequentially (this is valid after the patch in bug 887054 lands.

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: run the distribution test sequentially

There was a bug in the test harness. As said before, if test_browserGlue_distribution.js is run separately, everything runs.

That's why I used the run-sequentially option in the manifest. This is supported by the new harness.

Comment 5

[Marco Bonardo [:mak]]

Good catch!
The comment in the patch looks wrong, it states all of the browserglue test have a problem, not just the distribution test, could you please fix it?

What you says in comment 2 is correct, sadly there's no way to test distribution differently than by changing the application dir, though we may introduce one.
We could add an hidden pref that tells the component where to find the distribution.ini (maybe not a direct path, but like a boolean "browser.distribution.loadFromProfile", so we don't have to modify the appdir, we just copy our test file to the profile folder and set the pref.
The code loading the file is here http://mxr.mozilla.org/mozilla-central/source/browser/components/distribution.js#22 and changing the test is trivial. I can make the patch or review one, if you wish.

Comment 6

[Marco Bonardo [:mak]]

well, I think I will just make a patch, since it should not take a lot of time.

Comment 7

[Mihnea Dobrescu-Balaur (:mihneadb)]

I asked about a pref and I was told that is not desired. If you think it s ok, go for it!

Comment 8

[Marco Bonardo [:mak]]

not desired for what reason? and what kind of pref?

Comment 9

[Marco Bonardo [:mak]]

Also, who did you ask to?

Comment 10

[Marco Bonardo [:mak]]

Attachment: allow browserGlue to run parallel to other tests

This is the patch, quite trivial.

Now I suppose the best reviewer is the same person who told you to not use a pref, since there may be reasons behind that escaping me.
The only downside I see is that we don't test anymore that the distribution file is in "XREExeF", but that has never been the purpose of this test, and this is doing more computation than the one needed to do that, so if we want to retain a test for that that will run sequentially, I'd rather write a separate test that only does that, so we keep the parallel speed gain for this test that does far more. I don't think that actually matters fwiw, but again it's trivial to do.

Comment 11

[Mihnea Dobrescu-Balaur (:mihneadb)]

I don't remember, it was in #developers. I guess we can find out at review time.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 771472 [details] [diff] [review]
allow browserGlue to run parallel to other tests

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

Well, I would prefer to hit the right person, though in lack of that I will go for a more general approval from Gavin. The review should take just a few minutes.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 771472 [details] [diff] [review]
allow browserGlue to run parallel to other tests

I think I'd prefer calling the pref "distribution.testing.loadFromProfile" to better indicate that this is intended for test purposes only (and hopefully avoid people relying on this more widely).

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch

change the pref name as requested.

https://hg.mozilla.org/integration/mozilla-inbound/rev/280b4d55a2a5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/280b4d55a2a5