459114 - helper function to provide a clean profile directory for xpcshell tests

Status: VERIFIED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Shawn Wilsher :sdwilsh]

It sure would be nice for people to not need to use the boilerplate that is found everywhere for ProfD to work.  Additionally, we should make sure it's a clean ProfD for every test run so bad tests don't cause more failures that are difficult to debug.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

Having issues with the password manager tests here.  dolske is currently looking into it, but I have other things that I need to work on.

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v0.2

More work - forgot to qrefresh.

Basically, if we delete key3.db from dist/bin, and run the password manager tests everything works out fine.

If we run make check toplevel, things fail in the password manager.  Not really sure why either.

Comment 4

[(not currently active) Ted Mielczarek]

I want this functionality for a test I want to write, but I'd rather this be opt-in, since I'm not sure all tests will want it, and there might be tests that explicitly don't want a profile. I think we can just expose do_setup_profile_directory() and make it return the profile directory. That should work just fine. Might need a smarter way to cleanup at the end, as well.

Comment 5

[(no longer active)]

Ted, let me bring this to your attention as it might help.

In netwerk/test/unit/test_bug248970_cache.js, I setup a profile directory manually, but forgot to clean it up at the end of the test.  Bug 489585 was filed to clean up that directory, but my relatively simple patch there caused a crash at xpcshell cleanup on Mac (and only Mac).  You may want to look for similar problems in the cleanup code for this bug...

Comment 6

[(not currently active) Ted Mielczarek]

Ehsan: thanks for pointing that out. Because of that I decided to implement this with cooperation between the Python harness and the JS harness. The Python harness creates a temp dir prior to each test being run and sticks the path into an environment variable. The JS harness provides a do_get_profile() function that sets up the directory provider to point to that directory and also returns it as an nsILocalFile. The Python harness then removes the directory after xpcshell exits.

Comment 7

[(no longer active)]

Sounds like a very reasonable plan.  Thanks for driving this forward!

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: v0.3 [Checkin: See comment 18+19+20 & 28]

Ok, this implements what I just said I was implementing, and fixes all the tests I could find in the tree that were currently rolling their own dir provider for a profile dir to use it.

r?sdwilsh because I was a little more invasive on the DM tests--while testing the patch I noticed that since I made the changes to make tests not reference files from the srcdir, all the DM tests have been requesting a file that's 404 to the test webserver. I fixed that.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 377454 [details] [diff] [review]
v0.3
[Checkin: See comment 18+19+20 & 28]

You should be able to drop the directory provider in the download manager tests for DLoads.  We've dropped support for downloads.rdf migration on mozilla-central.

r=sdwilsh for download manager and places changes.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 377454 [details] [diff] [review]
v0.3
[Checkin: See comment 18+19+20 & 28]

>diff --git a/toolkit/mozapps/extensions/test/unit/head_extensionmanager.js b/toolkit/mozapps/extensions/test/unit/head_extensionmanager.js
>--- a/toolkit/mozapps/extensions/test/unit/head_extensionmanager.js
>+++ b/toolkit/mozapps/extensions/test/unit/head_extensionmanager.js
>@@ -234,8 +230,6 @@
>  */
> function shutdownEM()
> {
>-  // xpcshell calls xpcom-shutdown so we don't actually do anything here.
>-  gDirSvc.unregisterProvider(dirProvider);
>   gEM = null;
> }

Please leave the comment in the function. You can probably also remove the code from tail_extensionmanager.js that tries to delete the profile folder.

Comment 11

[(not currently active) Ted Mielczarek]

Waldo, when you get time to review this, you only really need to look at:
testing/xpcshell/head.js
testing/xpcshell/example/unit/test_profile.js
testing/xpcshell/runxpcshelltests.py

The rest of the patch is just changing existing tests to use the new helper function.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 377454 [details] [diff] [review]
v0.3
[Checkin: See comment 18+19+20 & 28]

Re-assigning review of the harness bits to sdwilsh, since he agreed to do them.

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 377454 [details] [diff] [review]
v0.3
[Checkin: See comment 18+19+20 & 28]

I'd prefer if you used a real java-doc style comment for do_get_profile().

r=sdwilsh with that change.

Comment 14

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/9ddc25fb2246

Comment 15

[Robert Strong (they/them - no direct email)]

I just tried

var gProfD = do_get_profile();
dump("profile path: " + gProfD + "\n");

in my head file and it failed on windows. It appears that XPCSHELL_TEST_PROFILE_DIR wasn't set. Perhaps a unit test to test this functionality would be a good thing?

Comment 16

[Robert Strong (they/them - no direct email)]

(In reply to comment #15)
> I just tried
> 
> var gProfD = do_get_profile();
> dump("profile path: " + gProfD + "\n");
was actually
dump("profile path: " + gProfD.path + "\n");

Comment 17

[(not currently active) Ted Mielczarek]

Oops, forgot to reopen. I just backed this out because it was failing tests. It's possible rebasing it broke it along the way.

Backout:
http://hg.mozilla.org/mozilla-central/rev/c83b6352eb12 (merge)
http://hg.mozilla.org/mozilla-central/rev/c29765db7521

Comment 18

[(not currently active) Ted Mielczarek]

It turned out to just be a bad merge from rebasing. I fixed it up, verified that it passed tests, and re-pushed:
http://hg.mozilla.org/mozilla-central/rev/e570c006d264

Comment 19

[(not currently active) Ted Mielczarek]

I made a bad change to one test, pushed a bustage fix:
http://hg.mozilla.org/mozilla-central/rev/eaaff4b3401c

Comment 20

[(not currently active) Ted Mielczarek]

*sigh* Fixed that test for real (and tested it on Windows):
http://hg.mozilla.org/mozilla-central/rev/884483e28aae

Turns out putting the profile directory in $TEMP broke that test, because it was saving the file to the profile directory by default, and I guess Windows doesn't like to put files from the temp directory in the recent files list.

Comment 21

[Serge Gautherie (:sgautherie)]

V.Fixed, per bug 489585 comment 24.

Comment 22

[Serge Gautherie (:sgautherie)]

Any reason not to land this bug on m-1.9.1?

Comment 23

[(not currently active) Ted Mielczarek]

You're welcome to land it there if you want, it's strictly test-harness+test fixes.

Comment 24

[Dave Townsend [:mossop]]

This landed before we branched for 1.9.2

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)
> This landed before we branched for 1.9.2

Indeed: not 1.9.3a1, not 1.9.2a1, but 1.9.2b1 ;->

Comment 26

[Shawn Wilsher :sdwilsh]

(In reply to comment #25)
> Indeed: not 1.9.3a1, not 1.9.2a1, but 1.9.2b1 ;->
No, we had a1 before we branched:
at Thu Aug 13 14:08:32 2009 +0100	GECKO_1_9_2_BASE
at Thu Aug 06 15:38:47 2009 -0700	FIREFOX_3_6a1_RELEASE

Comment 27

[(not currently active) Ted Mielczarek]

Plz stop quibbling about the target milestone of a patch to our test harnesses. It doesn't really matter.

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 377454 [details] [diff] [review]
v0.3
[Checkin: See comment 18+19+20 & 28]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b8f01b7aadc3
merged patch and 2 bustage fixes

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #26)
> (In reply to comment #25)
> > Indeed: not 1.9.3a1, not 1.9.2a1, but 1.9.2b1 ;->
> No, we had a1 before we branched:
> at Thu Aug 13 14:08:32 2009 +0100    GECKO_1_9_2_BASE
> at Thu Aug 06 15:38:47 2009 -0700    FIREFOX_3_6a1_RELEASE

Fyi, changeset dates are irrelevant, as they can be arbitrarily set.
(Only push dates are relevant.)

In my m-c repo:
FIREFOX_3_6a1_RELEASE = changeset:  31431 : da7fbe8a24dd
GECKO_1_9_2_BASE      = changeset:  31410 : 376b78fc7223
Main patch push       = changeset:  31160 : e570c006d264
FFv3.6a1 'default' r. = changeset:  31152 : 5c913c4662d8
In other words:
branching is irrelevant and this bug T.M. _is_ 1.9.2b1.

I would guess you looked at http://hg.mozilla.org/mozilla-central/summary,
but that reports the "last" push (only) of the branch, not its 'default' base :-/
(At least, I did make that mistake (too) previously :-<)