Closed Bug 459114 Opened 11 years ago Closed 11 years ago

helper function to provide a clean profile directory for xpcshell tests

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9.2b1

People

(Reporter: sdwilsh, Assigned: ted)

References

Details

(Whiteboard: [fixed1.9.1.4])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch v0.1 (obsolete) — Splinter Review
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.
Attached patch v0.2 (obsolete) — Splinter Review
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.
Attachment #342350 - Attachment is obsolete: true
Blocks: 466524
Assignee: sdwilsh → nobody
No longer blocks: 466524
Duplicate of this bug: 469829
Status: ASSIGNED → NEW
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.
Assignee: nobody → ted.mielczarek
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...
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.
Summary: Automagically provide a clean profile directory for tests → helper function to provide a clean profile directory for xpcshell tests
Sounds like a very reasonable plan.  Thanks for driving this forward!
Blocks: 489585
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.
Attachment #342367 - Attachment is obsolete: true
Attachment #377454 - Flags: review?(sdwilsh)
Attachment #377454 - Flags: review?(jwalden+bmo)
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.
Attachment #377454 - Flags: review?(sdwilsh) → review+
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.
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 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.
Attachment #377454 - Flags: review?(sdwilsh)
Attachment #377454 - Flags: review?(jwalden+bmo)
Attachment #377454 - Flags: review+
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.
Attachment #377454 - Flags: review?(sdwilsh) → review+
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/9ddc25fb2246
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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?
(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");
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
I made a bad change to one test, pushed a bustage fix:
http://hg.mozilla.org/mozilla-central/rev/eaaff4b3401c
*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.
V.Fixed, per bug 489585 comment 24.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Whiteboard: [needs m-1.9.2 and m-1.9.1 landings]
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
Attachment #377454 - Attachment description: v0.3 → v0.3 [Checkin: See comment 18+19+20]
Any reason not to land this bug on m-1.9.1?
You're welcome to land it there if you want, it's strictly test-harness+test fixes.
This landed before we branched for 1.9.2
Whiteboard: [needs m-1.9.2 and m-1.9.1 landings] → [needs m-1.9.1 landings]
Target Milestone: mozilla1.9.3a1 → mozilla1.9.2a1
(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 ;->
Keywords: checkin-needed
Target Milestone: mozilla1.9.2a1 → mozilla1.9.2b1
(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
Target Milestone: mozilla1.9.2b1 → mozilla1.9.2a1
Plz stop quibbling about the target milestone of a patch to our test harnesses. It doesn't really matter.
Attachment #377454 - Attachment description: v0.3 [Checkin: See comment 18+19+20] → v0.3 [Checkin: See comment 18+19+20 & 28]
(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 :-<)
Keywords: checkin-needed
Whiteboard: [needs m-1.9.1 landings] → [fixed1.9.1.4]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.2b1
Depends on: 518044
You need to log in before you can comment on or make changes to this bug.