476539 - use a XULRunner-based harness for unit testing

Status: RESOLVED FIXED

(Cloud Services :: General, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

Weave employs a number of hacks to run unit tests in xpcshell.  A XULRunner-based harness would require many fewer of those hacks.

It'd also give Weave some additional testing capabilities, like the ability to test code that uses profiles, for which Weave would otherwise need the heavier-weight MochiTest harness.

XULRunner is also easier to get, since it is built into Firefox 3+, so users wouldn't need to get the Gecko SDK in order to run tests.

So Weave should use a XULRunner-based harness for unit testing.

Comment 1

[Atul Varma [:atul]]

It might be useful to leverage QA's excellent mozrunner and jsbridge tools here, possibly MozMill as well.  Right now a few of Ubiquity's unit tests only run in a XULRunner (i.e., non-xpcshell) environment, and they're run using CI (buildbot) via jsbridge, passing back their success status to a host Python process.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

Here's a work in progress that converts Weave's unit test harness to use XULRunner.  I developed this harness to test both Weave and the modules in the JS Modules project.

In order to support multiple projects, all project-specific code is in the unit/ directory, while harness/ contains only generic harness code.  That leads to some duplication of code between unit/Makefile and harness/Makefile.  Perhaps there's a better way to handle that, though; my Makefile fu is not very high-level (although I've learned a lot hacking on this code).

I ripped out a few of the xpcshell-hacks, but I haven't ripped out all of them yet (like the fake services in unit/head.js that are probably no longer necessary).  Other issues still to resolve:

1. sometimes the first test fails when you start the tests without a pre-existing profile;
2. certain tests fail intermittently on Mac OS X
3. test_crypto_keypair fails on IWeaveCrypto.generateKeypair (other IWeaveCrypto methods work fine)
4. test_passwords fails because of bug 445873; but on builds with the fix, it fails for another reason

Note: some tests fail with both the xpcshell and the XULRunner harnesses.  I think they just need to be updated to work with recent code changes (f.e. bug 475855), although I haven't investigated fully yet.

Comment 3

[Dan Mills [:thunder]]

This is pretty cool, Myk.  Do you have any leads for issues 1-4? 2 and 3 are worrisome.  I'd love to switch to xlurunner-based tests if I could.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress 2: resolves issues 1 and 2

(In reply to comment #3)
> This is pretty cool, Myk.  Do you have any leads for issues 1-4? 2 and 3 are
> worrisome.

I think the first two issues are due to the way I was creating and specifying the profile directory.  XULRunner wants to restart after registering components, and I wasn't letting it.

I tried to fix that before by letting it restart, but I ran into a strange error that I've now figured out is unrelated (XULRunner on Mac doesn't accept relative paths for the -profile switch).

This version of the patch lets XULRunner restart after installing the extension.  It also lets XULRunner set up the profile/extensions directory instead of creating it manually, which might be a bit more robust.

With this version, so far I'm not seeing issues 1 and 2 anymore.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #4)
> (XULRunner on Mac doesn't accept relative paths for the -profile switch).

Erm, to be specific: it does accept relative paths, but something goes wrong when it tries to set up the profile at the path; I think it's that the extensions manager isn't able to create extensions.rdf.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress 3: |make clean| improvements

This work in progress is the same as the last one except that |make clean| in the top level directory now also invokes |make clean| in tests/unit/; |make clean| in tests/unit/ removes the log files too; and profile directory removal is no longer verbose (for consistency with other cleansing commands).

I've done some more testing, and I'm continuing to see consistent behavior on tests now, so I think issues 1 and 2 are solved by the way the patch now creates the profile directory, installs the extension, and then lets XULRunner register the extension, restarting if necessary.

Issue 4 is because test_passwords is out of date, as are other tests.  Here's the current list of tests that are failing (and a key to why; see below for detailed explanations):

test_load_module_engines_slash_cookies (syncCores.js)
test_load_module_engines_slash_forms (syncCores.js)
test_load_module_engines_slash_input (syncCores.js)
test_load_module_engines_slash_passwords (syncCores.js)
test_load_module_engines_slash_tabs (syncCores.js)
test_passwords (bug 445873, syncCores.js)
test_records_keys ("pubkey.lastRequest is undefined")
test_records_wbo ("res.get is not a function")
test_crypto_keypair (IWeaveCrypto.generateKeypair NS_ERROR_FAILURE)
test_records_crypto (IWeaveCrypto.generateKeypair NS_ERROR_FAILURE)

Of these, the test_load_* tests all fail because they import engine modules that try to import the syncCores.js module, which doesn't exist anymore (I'd cite the removal changeset here, but I haven't been able to figure out how to get Mercurial to tell me that).  They fail the same way with the xpcshell harness.

test_passwords fails because of bug 445873 on builds of XULRunner without the fix for that bug; on builds with the fix, it fails because it too loads an engine module that tries to import syncCores.js.  It fails in the latter way with the xpcshell harness.

test_records_keys fails because "pubkey.lastRequest is undefined", which sounds like an out-of-date test problem (like the ones I fixed in bug 475855).  It fails the same way with the xpcshell harness.

test_records_wbo fails on "res.get is not a function".  It fails the same way with the xpcshell harness.

test_crypto_keypair and test_records_crypto both fail when they call IWeaveCrypto.generateKeypair, which reports NS_ERROR_FAILURE.  This is the only test that behaves differently between the two harnesses, and I have no idea why.  Other IWeaveCrypto functions work just fine, and the error happens on all three platforms on which I'm testing (Mac OS X 10.5, Ubuntu 8.04, Windows Vista).  It also happens with multiple versions of XULRunner, including 1.9.0 stable releases and 1.9.1 nightlies.

If we could figure out that issue (issue 3 on the earlier list), this harness would be ready to go.

Some unrelated fixes that have snuck their way into this patch:

* the "\t" in PASS/FAIL messages echoed to the console is now interpreted correctly on Linux and Windows.  Before, it would appear as the literal characters "\t", since "echo" on those platforms needs the |-e| switch in order to interpret backslash escapes.
* NATIVE_TOPSRCDIR is set correctly for Windows in the test/harness/ Makefile.  Before it contained a forward slash instead of a backslash.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Some digging reveals that test_crypto_keypair (and probably also test_records_crypto) is dying because NSS (PK11_GenerateKeyPairWithOpFlags) is returning SEC_ERROR_TOKEN_NOT_LOGGED_IN.

Comment 8

[Dan Mills [:thunder]]

dveditz, kaie, any advice here?

Comment 9

[Justin Dolske [:Dolske]]

I've had problems, through password manager, where NSS/PSM seems to unexpectedly think that the user has logged out from the token. See bug 437904.

You might be able work around this by having WeaveCrypto::GenerateKeypair() call PK11_GenerateKeyPairWithOpFlags() instead of PK11_GenerateKeyPair()... The latter generates a key with CKA_PRIVATE set, which requires the token to be logged into. Weave doesn't need that, it's only using this API (instead of the WithOpFlags flavor) because it's a bit simpler simpler to use.

Comment 10

[Justin Dolske [:Dolske]]

I suppose by "you" I mean "I would be willing to write this patch". :)

Comment 11

[Dan Mills [:thunder]]

Justin: I offer you some delicious bacon in exchange for your patch ;)

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: switches Weave to XULRunner-based harness

Here's a version of the patch updated to apply cleanly to the latest Weave tip.  It also contains a few minor improvements to the harness from the jsmodules version of it.

Assuming dolske's fix for bug 479341 resolves the issue with test_crypto_keypair and test_records_crypto, this should be ready for review, although I'll wait to request review until we confirm dolske's work resolves the problem.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: includes harness app's chrome.manifest file

Weave's .hgignore file says to ignore chrome.manifest files.  The test harness application has one of those, but it wasn't being included in the patch because of the .hgignore rule.  This patch contains it.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: erm, v2 without the extra unnecessary stuff

Comment 15

[Dan Mills [:thunder]]

(In reply to comment #13)
> Weave's .hgignore file says to ignore chrome.manifest files.

It should only ignore the toplevel chrome.manifest, because it's the only one generated from a .in file.

From http://www.selenic.com/mercurial/hgignore.5.html it looks like

^chrome.manifest$

Will do that.

Comment 16

[Dan Mills [:thunder]]

(In reply to comment #15)
> It should only ignore the toplevel chrome.manifest, because it's the only one
> generated from a .in file.

Pushed:

http://hg.mozilla.org/labs/weave/rev/ce39314a4744

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: erm, v3 with one extra necessary thing

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: doesn't attempt to set window.crypto; removes modules/crypto.js

Over in bug 479341, dolske fixed WeaveCrypto::GenerateKeypair, which fixed the issue that test_crypto_keypair and test_records_crypto had with that method, but test_records_crypto was still failing with:

TypeError: setting a property that has only a getter
...
*** CHECK FAILED: TypeError: crypto.generateRandomKey is not a function

The problem is that the test tries to set a global |crypto| variable to an instance of the IWeaveCrypto service, but now that the test is running in a window, window.crypto is already defined <https://developer.mozilla.org/En/DOM/Window.crypto> and can't be set.

This version of the patch fixes that problem by retrieving the service into a local variable where it's used in async_test.

With this patch, the test still fails with:

*** CHECK FAILED: TypeError: this.payload is undefined

However, that also happens with the old harness, as the test has bitrotted.


There's one more difference in the test results that has cropped up since I originally compared the two harnesses.  test_load_module_crypto succeeds on the old harness but fails on the new one.

It turns out that modules/crypto.js is obsolete and should be removed.  Because the new harness actually imports it, it triggers instantiation of the Crypto object it exports, which runs some obsolete code that throws.  The old harness tested it by loading it in a sandbox, which doesn't trigger instantiaton of the Crypto object.

Thus the new harness is the one generating the correct result for that test.  But the module should be removed, so this version of the patch does that.


With dolske's fix in bug 479341 and the removal of modules/crypto.js, this version of the XULRunner harness returns identical results to the old xpcshell harness and is ready for review.

Comment 19

[Dan Mills [:thunder]]

Comment on attachment 367704 [details] [diff] [review]
patch v5: doesn't attempt to set window.crypto; removes modules/crypto.js

very cool stuff!

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Fixed by http://hg.mozilla.org/labs/weave/rev/0905d37db078.

This means that making Weave now requires you to define a XULRunner binary in addition to the XULRunner SDK directory.  Do so via the make command variable xulrunner_bin or the environment variable XULRUNNER_BIN:

  make xulrunner_bin=/foo/bar/xulrunner
  export XULRUNNER_BIN=/foo/bar/xulrunner; make

The XULRunner binary can be an instance of Firefox 3.0 or newer (except where Firefox is a XULRunner application, like on recent distributions of Ubuntu, in which case you have to specify the XULRunner binary that Firefox runs on top of).  Here are some common XULRunner binaries:

Linux:
  /usr/bin/firefox
  /usr/bin/xulrunner

Mac OS X:
  /Library/Frameworks/XUL.framework/xulrunner-bin
  /Applications/Firefox.app/Contents/MacOS/firefox

Windows (MozillaBuild):
  /c/Program\ Files/Mozilla\ Firefox/firefox.exe

Note: the binary that comes with the Mac OS X universal XULRunner SDKs that you build using the weave/scripts/build-universal-sdk.sh script doesn't run the test harness for some reason (yet to be determined).  So don't use that one to run the tests (although do continue to use it to build Weave).