382752 - Give extension manager some xpcshell test love

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Alex Vincent [:WeirdAl]]

I've been thinking for a while that with some Makefile hacking and reusing the XUL preprocessor on a single install.rdf file, we could generate a few test XPI packages.  With those packages, we could start writing xpcshell tests for the extension manager.

toolkit/mozapps/extensions/tests/Makefile.in

#ifdef ENABLE_TESTS
libs::
# xul preprocessor for toolkit/mozapps/extensions/tests/files/install.rdf -> dist/xpi-stage/test-em-one/install.rdf
# makexpi packages to _tests/xpcshell-simple/test_extmgr/em-test-one.xpi
#endif

MODULE		= test_extmgr

XPCSHELL_TESTS  = unit

toolkit/mozapps/extensions/tests/files/ would hold the source files for several extensions, but only specified files would be preprocessed and included into each XPI.

Thoughts?

Comment 1

[Dave Townsend [:mossop]]

Coincidentally I started knocking together unit testing for the EM a couple of days ago and I now have a working unit test for bug 257155. We don't actually need to preprocess the install.rdf but I hadn't yet integrated converting an install.rdf to a usable xpi yet, I'll take that on board.

Comment 2

[Alex Vincent [:WeirdAl]]

I figured with preprocessing, we could have a single set of files and generate a bunch of different XPI's from them, thus keeping the number of base files to a minimum.  :)

Comment 3

[Dave Townsend [:mossop]]

Not totally sure I follow, can you give me an example of what different xpi's you might want to generate from one set of files?

Comment 4

[Alex Vincent [:WeirdAl]]

One case would be for testing updates - each extension in the test would have a different version number, and point to a local update.rdf manifest.

Test, part 1: Install the "older" XPI.
Test, part 2: Try to update to the "newer" XPI.
Test, part 3: Uninstall the extension.

Comment 5

[Alex Vincent [:WeirdAl]]

Another example would be attachment 248804 [details] (from bug 299716), testing [e-mail] extensions.

Comment 6

[Dave Townsend [:mossop]]

Attachment: unit test environment rev 1

This is the first hash at an environment enabling unit testing of the EM. Overview of what it provides:

Function to define the app version info for the test run.
Functions to startup, shutdown and restart the EM.
Utility functions for rdf access.
Creates a temporary profile dir.
Automatically converts any directories found in mozilla/toolkit/mozapps/extensions/test/unit/addons into xpi's that can be retrieved from a function in the testcase.

I will shortly attach a second patch that contains testcases as examples of usage.

Comment 7

[Dave Townsend [:mossop]]

Attachment: example testcases

These are example testcases for bugs 257155 and 340219.

Comment 8

[Dave Townsend [:mossop]]

Attachment: unit test environment rev 2

Minor update, no real changes but I have removed the usage of Cc and Ci. The http test server defines it itself so they conflict.

Comment 9

[Dave Townsend [:mossop]]

Attachment: testcase for bug 314915

This is another example testcase that utilises the js http server in order to test the update checker.

Note that update checking in the xpcshell environment (in fact any xmlhttprequest use in the EM) is currently broken due to bug 319968.

Also I've spotted that the unit test framework is currently converting the CVS directory to an xpi but I'll fix that in the next round.

Comment 10

[Alex Vincent [:WeirdAl]]

Mossop, it's worth asking if we should wait for bug 319968 to get fixed before we land this - or if we can just stick in a few instanceof calls into the extmgr to move things along.

Comment 11

[Alex Vincent [:WeirdAl]]

(In reply to comment #8)
> Created an attachment (id=267598) [details]
> unit test environment rev 2

This patch breaks if there are no tests included:

mkdir -p /c/trunk/fx-debug/_tests/xpcshell-simple/test_extensionmanager/unit/addons
for dir in /c/trunk/mozilla/toolkit/mozapps/extensions/test/unit/addons/*; do \
                base=`basename $dir` ; \
                cd $dir ; \
                zip /c/trunk/fx-debug/_tests/xpcshell-simple/test_extensionmanager/unit/addons/$base.xpi * ; \
        done
/bin/sh: cd: /c/trunk/mozilla/toolkit/mozapps/extensions/test/unit/addons/*: No such file or directory
zip I/O error: Invalid argument

zip error: Could not create output file (c:/trunk/fx-debug/_tests/xpcshell-simple/test_extensionmanager/unit/addons/*.xpi)
make[8]: *** [libs] Error 15

Comment 12

[Dave Townsend [:mossop]]

Attachment: unit test environment rev 3

This is a slightly updated test environment to counter some issues I came across with a couple of new testcases.

Differences are that it now handles there being nothing in the addons dir and doesn't try to make an xpi out of the CVS dir.

Enables the EM logging pref for a bit more useful information in the event of a failure.

Disables the browser disk cache which stops us from deleting the temporary profile folder at the end.

Comment 13

[Dave Townsend [:mossop]]

Attachment: Testcase for bug 335238

This is a testcase ostensibly for bug 335238. What it does is run an update check for two addons ensuring that the variables in their update url's are correctly changed to the appropriate values.

Comment 14

[Dave Townsend [:mossop]]

Attachment: Testcase for compatibility updates

This is for no bug in particular, but it tests that the EM correctly checks the update rdf for an addon that appears incompatible at first glance. This attempts to install 2 extensions both of which are incompatible according to the install.rdf. It then waits a few seconds for the phone home and then restarts and verifies that one of the extensions was correctly installed and the other not.

Comment 15

[Alex Vincent [:WeirdAl]]

Comment on attachment 267984 [details] [diff] [review]
unit test environment rev 3

A few drive-by comments, based on my tinkering with this patch:

>Index: toolkit/mozapps/extensions/test/unit/head_extensionmanager.js
>+/**
>+ * Returns a testcase xpi
>+ * @param   name
>+ *          The name of the testcase (without extension)
>+ * @returns an nsILocalFile pointing to the testcase xpi
>+ */
>+function do_get_addon(name)
>+{
>+  var lf = Components.classes["@mozilla.org/file/directory_service;1"]
>+                     .getService(Components.interfaces.nsIProperties)
>+                     .get("CurWorkD", Components.interfaces.nsILocalFile);
>+  lf = lf.parent.parent.parent.parent;
>+  lf.append("_tests");
>+  lf.append("xpcshell-simple");
>+  lf.append("test_extensionmanager");
>+  lf.append("unit");
>+  lf.append("addons");
>+  lf.append(name + ".xpi");
>+  if (!lf.exists())
>+    do_throw("Addon "+name+" does not exist.");
>+
>+  return lf;
>+}

This is a long, lengthy process just to get where the XPI's are, in the _tests directory.  There has to be a better way, or perhaps the test_all.sh, test_one.sh files need to pass another environment variable into xpcshell.  It'd be nice if you were able to get the parent directory of the XPI's, and then we clone that and append the XPI name.

>+// Need to create and register a profile folder.
>+var gDirSvc = Components.classes["@mozilla.org/file/directory_service;1"]
>+                        .getService(Components.interfaces.nsIProperties);
>+var tmpDir = gDirSvc.get(NS_OS_TEMP_DIR, Components.interfaces.nsIFile);
>+var gProfD = tmpDir.clone();
>+gProfD.append("mozprofile");

...

There's actually a subtle design flaw here, and I'm not sure it's entirely yours.  Imagine I'm running this code under check-interactive.  What happens is the head and tail files get loaded right away, and then the user manually calls _execute_test().  The result?

>Index: toolkit/mozapps/extensions/test/unit/tail_extensionmanager.js
>+// Close down any remaining EM
>+if (gEM)
>+  shutdownEM();
>+
>+// Clean up the temporary profile dir.
>+if (gProfD.exists())
>+  gProfD.remove(true);

This code executes before the test does.  That means the profile directory gets blown away, and the conditions for the test are no longer constant.

One solution to this would be to wrap this code in a function that run_test or one of its subsidiaries calls.  A better one would be to define in head.js a couple arrays to store functions that _execute_test() must call on.

Comment 16

[Alex Vincent [:WeirdAl]]

Comment on attachment 267988 [details] [diff] [review]
Testcase for compatibility updates

Drive-by comments, continued:

>Index: toolkit/mozapps/extensions/test/unit/addons/test_incompatible_1/install.rdf
>+    <em:updateURL>http://localhost:4444/test_incompatible_1.rdf</em:updateURL>

Can you add a check to see if the updateURL is properly set on the update item object after the extmgr restart?  I've been playing with code very similar to this, and I can't verify that.

>Index: toolkit/mozapps/extensions/test/unit/test_incompatible.js
>+var server;

I just noticed, actually, that httpd.js defines its own server value; in fact, it's a constructor function for xpcshell-oriented HTTP servers:
http://lxr.mozilla.org/seamonkey/source/netwerk/test/httpserver/httpd.js#2777

>+  // Give time for phone home to complete
>+  do_timeout(2000, "delayed_run()");

I think this is wrong.  It's an assumption that we'll need at most 2 seconds to process everything.  You might want to look into the thread manager code inside the server constructor I mention above.

With the proper patches applied, there's several of these warnings inside the log file:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Failure' when calling method: [nsIDirectoryServiceProvider::getFile]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_extensionmanager/unit/head_extensionmanager.js :: startupEM :: line 239"  data: no]
************************************************************

I believe it's firing for "CurProcD" - at least, that's what I was seeing earlier when I went to debug it.

Plus one of these:

###!!! ASSERTION: Component Manager being held past XPCOM shutdown.: 'cnt == 0', file c:/mozilla.org/trunk/mozilla/xpcom/build/nsXPComInit.cpp, line 826

Assertion failures will cause the test to fail on trunk.

Comment 17

[Alex Vincent [:WeirdAl]]

(In reply to comment #16)
> Can you add a check to see if the updateURL is properly set on the update item
> object after the extmgr restart?  I've been playing with code very similar to
> this, and I can't verify that.

Now I can; please ignore the above comment.

Also, I am getting this whenever I call gEM.update() for my test extensions:

[Exception... "'[JavaScript Error: "gPref has no properties" {file: "file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsUpdateService.js" line: 2686}]' when calling method: [nsIUpdateTimerManager::registerTimer]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsExtensionManager.js :: anonymous :: line 2998"  data: yes]

Comment 18

[Dave Townsend [:mossop]]

For the getting the xpi's yes it'd be nice to reduce that somewhat, I'll bug sayrer for his thoughts.

In the check-interactive case I believe it's an issue with how check-interactive works. My understanding is that the tail script is supposed to be run after the test is complete, so if check-interactive is running it before the test then I'd say there's a bug there.

I haven't yet figured out a particular way to get rid of the timeout from that test, yes it's a bit nasty.

For the directory service providers errors, I was under the impression that dir service providers were meant to throw when an unknown key was requested, looks like this may not be the case though so I'll tidy that up.

I don't see the assertion or gPref errors, you had better provide me with a copy of your test so I can investigate those.

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 267984 [details] [diff] [review]
unit test environment rev 3

Cancelling review, there are a few issues still to be sorted out here.

Comment 20

[Dave Townsend [:mossop]]

Attachment: unit test environment rev 4

This is a work in progress, not convinced I'm happy with it yet.

The main problem with the EM is that it uses some variables global to it's component scope, which it deletes when you shut it down. So without changing the EM component itself we cannot do a full shutdown and restart of it. Thankfully this turns out to be not a major issue (at the moment). Shutting down only cleans up variables and observers, it does not actual work so the procedure now is to merely recall the startup sequence when we want to simulate a restart. This has the benefit that we can use the EM component as a service again properly.

There are a number of assertions that show up in this in debug builds, I know that a couple are not caused by the test harness but by something else. At least one occurs in every xpcshell test I've seen in a debug build. A couple of others I'm not sure of. The main problem right now is that the temporary profile folder created for the test run is failing to be deleted in some cases so something is holding a file in the profile folder open.

Comment 21

[Dave Townsend [:mossop]]

Note to self, the things holding the profile folder open appear to be the js components fast load cache file and the cookies database.

Comment 22

[Alex Vincent [:WeirdAl]]

Something very funny going on in my usage.  I do a restart (and I try to call gEM.update()), but there's no indication that says either of these actually tries to get the new XPI.

When I set my update.rdf's em:version to 0.2 (to trigger an update), in onDatasourceLoaded I end up with:

newerItem: undefined
sameItem: null
item: ({id:"[e-mail]">[e-mail]", version:"0.1", installLocationKey:"app-profile", minAppVersion:"1", maxAppVersion:"1", name:"Bug 299716 test A", xpiURL:"", xpiHash:"", iconURL:"", updateRDF:"http://localhost:4444/data/test_bug299716_a.rdf", type:2, targetAppID:(void 0)})
status: 8

This is just as RDFItemUpdater.checkForUpdates() fires off its request.

Also, this check fails:

function run_test_pt2() {
  var extensions = gProfD.clone();
  extensions.append("extensions");
  do_check_true(extensions.exists());
  var ioServ = Components.classes["@mozilla.org/network/io-service;1"]
                         .getService(Components.interfaces.nsIIOService);
  var addons = extensions.directoryEntries;
  do_check_true(addons.hasMoreElements());
  // ...
}

This suggests to me that extensions staged for an update don't live here.  Where they live I don't know.

Comment 23

[Alex Vincent [:WeirdAl]]

Disregard comment 22, my results were bogus.

Comment 24

[Alex Vincent [:WeirdAl]]

Unofficial r+ from me; I can finally do a complete extension install and update using this harness.  Also, I don't see anything wrong with the patch.

However, I'm catching a few NS_ASSERTION() failures when running my test.  As a result, I've set XPCOM_DEBUG_BREAK=stack within my test file.

###!!! ASSERTION: nsStandardURL not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file c:/mozilla.org/trunk/mozilla/netwerk/base/src/nsStandardURL.cpp, line 931
 !nsCOMPtr<nsIURI>::~nsCOMPtr<nsIURI>+0x000000000000003B
 !nsInstallInfo::~nsInstallInfo+0x000000000000003D
 !nsInstallInfo::`scalar deleting destructor'+0x000000000000000F
 !nsSoftwareUpdate::InstallJarCallBack+0x000000000000005B
 !RunInstallOnThread+0x00000000000004B8
 !_PR_NativeRunThread+0x00000000000000DB
 !pr_root+0x0000000000000019
 !beginthreadex+0x0000000000000221
 !beginthreadex+0x00000000000001C7
 !GetModuleFileNameA+0x00000000000001B4

###!!! ASSERTION: nsStandardURL not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file c:/mozilla.org/trunk/mozilla/netwerk/base/src/nsStandardURL.cpp, line 931
 !nsCOMPtr<nsIURL>::~nsCOMPtr<nsIURL>+0x000000000000003B
 !nsJARURI::~nsJARURI+0x0000000000000044
 !nsJARURI::`scalar deleting destructor'+0x000000000000000F
 !nsJARURI::Release+0x0000000000000093
 !nsCOMPtr<nsIURI>::~nsCOMPtr<nsIURI>+0x000000000000003B
 !nsInstallInfo::~nsInstallInfo+0x0000000000000048
 !nsInstallInfo::`scalar deleting destructor'+0x000000000000000F
 !nsSoftwareUpdate::InstallJarCallBack+0x000000000000005B
 !RunInstallOnThread+0x00000000000004B8
 !_PR_NativeRunThread+0x00000000000000DB
 !pr_root+0x0000000000000019
 !beginthreadex+0x0000000000000221
 !beginthreadex+0x00000000000001C7
 !GetModuleFileNameA+0x00000000000001B4
### ERROR: WalkStack64: The parameter is incorrect.

###!!! ASSERTION: nsStandardURL not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file c:/mozilla.org/trunk/mozilla/netwerk/base/src/nsStandardURL.cpp, line 931
 !nsCOMPtr<nsIURI>::~nsCOMPtr<nsIURI>+0x000000000000003B
 !nsJARURI::~nsJARURI+0x000000000000004F
 !nsJARURI::`scalar deleting destructor'+0x000000000000000F
 !nsJARURI::Release+0x0000000000000093
 !nsCOMPtr<nsIURI>::~nsCOMPtr<nsIURI>+0x000000000000003B
 !nsInstallInfo::~nsInstallInfo+0x0000000000000048
 !nsInstallInfo::`scalar deleting destructor'+0x000000000000000F
 !nsSoftwareUpdate::InstallJarCallBack+0x000000000000005B
 !RunInstallOnThread+0x00000000000004B8
 !_PR_NativeRunThread+0x00000000000000DB
 !pr_root+0x0000000000000019
 !beginthreadex+0x0000000000000221
 !beginthreadex+0x00000000000001C7
 !GetModuleFileNameA+0x00000000000001B4
### ERROR: WalkStack64: The parameter is incorrect.

###!!! ASSERTION: nsChromeRegistry not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file c:/mozilla.org/trunk/mozilla/chrome/src/nsChromeRegistry.cpp, line 438
 !nsCOMPtr_base::assign_assuming_AddRef+0x0000000000000059
 !nsCOMPtr_base::assign_with_AddRef+0x000000000000002C
 !nsCOMPtr<nsISupports>::operator=+0x0000000000000012
 !nsProxyEventObject::~nsProxyEventObject+0x0000000000000033
 !nsProxyEventObject::`scalar deleting destructor'+0x000000000000000F
 !nsProxyEventObject::Release+0x00000000000000A0
 !nsXPTCStubBase::Release+0x0000000000000017
 !nsCOMPtr<nsIToolkitChromeRegistry>::~nsCOMPtr<nsIToolkitChromeRegistry>+0x000000000000003B
 !nsInstallInfo::~nsInstallInfo+0x0000000000000053
 !nsInstallInfo::`scalar deleting destructor'+0x000000000000000F
 !nsSoftwareUpdate::InstallJarCallBack+0x000000000000005B
 !RunInstallOnThread+0x00000000000004B8
 !_PR_NativeRunThread+0x00000000000000DB
 !pr_root+0x0000000000000019
 !beginthreadex+0x0000000000000221
 !beginthreadex+0x00000000000001C7
 !GetModuleFileNameA+0x00000000000001B4
### ERROR: WalkStack64: The parameter is incorrect.

Comment 25

[Alex Vincent [:WeirdAl]]

I found yet another bug in the test harness, using the provided prompt service.  The word "Minefield" probably should not be there:

alert: Bug 299716 test F 0.1 could not be installed because it is not compatible with Minefield 5. (Bug 299716 test F 0.1 will only work with Minefield 30)

Comment 26

[Alex Vincent [:WeirdAl]]

Correction: the bug isn't in the harness, but is bug 299716 comment 44.

Comment 27

[Dave Townsend [:mossop]]

Attachment: unit test environment rev 5

This is the next revision. In order to get around being unable to remove the profile folder on shutdown instead the profile folder is created in the _tests directory space at a single location that is cleared on startup.

I think this is close to done, the only issue I have is to try to avoid the horrible hack that is there for finding the _tests directory.

Comment 28

[Dave Townsend [:mossop]]

Attachment: unit test environment rev 6

This has a slightly less nasty way of choosing a location for the profile folder etc.

Comment 29

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

Comment on attachment 274137 [details] [diff] [review]
unit test environment rev 6

>+/**
>+ * Creates an nsIXULAppInfo
>+ * @param   id
>+ *          The ID of the test application
>+ * @param   name
>+ *          A name for the test application
>+ * @param   version
>+ *          The version of the application
>+ */
>+function createAppInfo(id, name, version)
>+{
>+  var XULAppInfo = {
>+    vendor: "Mozilla",
>+    name: name,
>+    ID: id,
>+    version: version,
>+    appBuildID: "2007010101",
>+    platformVersion: "1.9",
As we discussed platformVersion should also be a param

Comment 30

[Dave Townsend [:mossop]]

Attachment: final patch ready for checkin

This is the final version of the test harness, however will need at least one testcase to checkin along with it or tests fail (yay!)

Comment 31

[Alex Vincent [:WeirdAl]]

Comment on attachment 275993 [details] [diff] [review]
final patch ready for checkin

>Index: toolkit/mozapps/extensions/test/Makefile.in
>+TESTROOT = $(shell cd $(DEPTH) && pwd)/_tests/xpcshell-simple/$(MODULE)

...

>Index: toolkit/mozapps/extensions/test/unit/head_extensionmanager.js
>+function do_get_addon(name)
>+{
>+  var lf = gTestRoot.clone();
>+  lf.append("unit");
>+  lf.append("addons");
>+  lf.append(name + ".xpi");
>+  if (!lf.exists())
>+    do_throw("Addon "+name+" does not exist.");
>+
>+  return lf;
>+}

...

>+var gTestRoot = gDirSvc.get("CurWorkD", Components.interfaces.nsILocalFile);
>+gTestRoot.appendRelativePath(Components.stack.filename);
>+gTestRoot = gTestRoot.parent.parent;

These do not agree on Windows.  TESTROOT points to _tests\xpcshell-simple\test_extensionmanager\unit\addons\, lf points to toolkit\mozapps\extensions\test\unit\addons\.

Comment 32

[Dave Townsend [:mossop]]

Attachment: fixed for windows

This is a minor alteration to finding the tests directory that should work everywhere. Checked in.

Checking in toolkit/mozapps/extensions/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/Makefile.in,v  <--  Makefile.in
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.234; previous revision: 1.233
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/Makefile.in,v
done
Checking in toolkit/mozapps/extensions/test/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/head_extensionmanager.js,v
done
Checking in toolkit/mozapps/extensions/test/unit/head_extensionmanager.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/head_extensionmanager.js,v  <--  head_extensionmanager.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/tail_extensionmanager.js,v
done
Checking in toolkit/mozapps/extensions/test/unit/tail_extensionmanager.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/tail_extensionmanager.js,v  <--  tail_extensionmanager.js
initial revision: 1.1
done

Comment 33

[Dave Townsend [:mossop]]

This has landed now and all unit testers passed the one test I checked in. Will track individual testcases in the appropriate bugs from now.