759664 - Add support for test modules to mochi and mochi chrome

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Bug 748490 added support for "test modules" - see http://gregoryszorc.com/blog/2012/05/10/better-sharing-of-test-code-in-mozilla-projects/ for more details.  It would be great if support for this could be added to mochi.

For reference, the xpcshell code that does this magic is at http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#284

Comment 1

[(not currently active) Ted Mielczarek]

I'm not sure if this makes sense for Mochitest plain, since those tests are web content. It certainly makes sense for chrome Mochitests and browser-chrome tests, though.

Comment 2

[Gregory Szorc [:gps]]

AITC would really like this, as some code in services/common is only being exposed as a testing-only JS module.

How much work does this bug entail? If someone could spend a few minutes telling me what needs changed, I could possibly work on it. I just have no clue how the mochitest runner works and I think reading the code will take much longer than someone else giving me an estimate or pointers.

If we can't get this soon, I can probably hack around things until this is in place.

Comment 3

[Gregory Szorc [:gps]]

Ignore my last comment. We're going to work around it in bug 777077 (unless this bug resolves itself in the next day or two).

Comment 4

[Gregory Szorc [:gps]]

It turns out my workaround in bug 77077 isn't as trivial as I would have hoped: it would require changing how httpd.js is packaged.

Would it be possible to accomplish this bug via manifest changes? I /think/ if we add the appropriate line in the right place inside a .manifest file then resource://testing-common/ automagically gets registered. However, I'm not sure about the implications for packaging. We probably don't want production builds pointing to a local filesystem path containing testing-only JS files. We also don't want the testing-only JS files to be in dist/.

I see testing/mochitest/runtests.py is dynamically creating something that looks like a manifest file. Perhaps if we alter the test runner to add the appropriate content to that manifest things will just work?

Comment 5

[Gregory Szorc [:gps]]

Attachment: Register testing modules in manifest, v1

This registers the "testing-common" resource when mochitests are executed via the Python test runner.

I added a simple test to some necko code. I tried to create a new test file. But, I've never written a mochitest before and the state of the docs and the output from the test runner colluded caused me to give up after many failures. I concede that embedding the module loading inside the existing test doesn't feel right.

Try up at http://tbpl.mozilla.org/?tree=Try&rev=e3d19916bd53. I expect it to fail because this feature requires a new argument to the Python test runner and I'm pretty sure providing that argument will require buildbotcustom updates.

We could do the same thing we did with the xpcshell runner and try to guess the modules path. I'll probably put that in if it is required (do to Try failure) because I don't want to update buildbotcustom again.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Register testing modules in manifest, v2

As I kinda of expected, the original patch failed on Try. Why, oh why don't the buildbot configs live in the tree?

Anyway, this patch adds the same hack we added for the xpcshell runner. If the testing modules directory isn't defined, we assume we are inside a subdirectory of an expanded test package and look for it as a sibling directory. This allows the directory to be discovered on the buildbot hosts.

Try at http://tbpl.mozilla.org/?tree=Try&rev=e23813a12c9a. I expect it to pass this time.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 646000 [details] [diff] [review]
Register testing modules in manifest, v2

Try build failed. Back to the drawing board.

Comment 8

[Gregory Szorc [:gps]]

I've got a Try with tons of debug logs going at http://tbpl.mozilla.org/?tree=Try&rev=9ebec3015939. I also tweaked it and I'm slightly optimistic the tweak will make it work. I'll submit for review once I have a green Try run.

Comment 9

[Gregory Szorc [:gps]]

Attachment: Register testing modules in manifest, v3

A version of this patch with lots of debug statements passed Try!

Try with this exact patch up at http://tbpl.mozilla.org/?tree=Try&rev=159ed2f04f5d.

Comment 10

[Gregory Szorc [:gps]]

I had a bad patch in my queue before this one. Old try cancelled. New try at http://tbpl.mozilla.org/?tree=Try&rev=ff9a1a04166b

Comment 11

[Gregory Szorc [:gps]]

Try run was green.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 648032 [details] [diff] [review]
Register testing modules in manifest, v3

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

::: netwerk/test/browser/browser_NetUtil.js
@@ +9,5 @@
>    waitForExplicitFinish();
>  
> +  // We overload this test to include verifying that httpd.js is
> +  // importable as a testing-only JS module.
> +  Components.utils.import("resource://testing-common/httpd.js", {});

Maybe sanity-check a property of the imported module as well?

Comment 13

[Gregory Szorc [:gps]]

(In reply to Ted Mielczarek [:ted] from comment #12)
> ::: netwerk/test/browser/browser_NetUtil.js
> @@ +9,5 @@
> >    waitForExplicitFinish();
> >  
> > +  // We overload this test to include verifying that httpd.js is
> > +  // importable as a testing-only JS module.
> > +  Components.utils.import("resource://testing-common/httpd.js", {});
> 
> Maybe sanity-check a property of the imported module as well?

There is already an xpcshell test that does this. I think the module import should be a sufficient check in the mochitest test.

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8f3e2803cf

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5b8f3e2803cf