Add support for test modules to mochi and mochi chrome

RESOLVED FIXED in mozilla17

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: markh, Assigned: gps)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
Depends on: 757460
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.
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.
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).
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?
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.
Attachment #645583 - Flags: feedback?(ted.mielczarek)
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.
Attachment #645583 - Attachment is obsolete: true
Attachment #645583 - Flags: feedback?(ted.mielczarek)
Attachment #646000 - Flags: review?(ted.mielczarek)
Comment on attachment 646000 [details] [diff] [review]
Register testing modules in manifest, v2

Try build failed. Back to the drawing board.
Attachment #646000 - Flags: review?(ted.mielczarek)
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.
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.
Assignee: nobody → gps
Attachment #646000 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #648032 - Flags: review?(ted.mielczarek)
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
Try run was green.
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?
Attachment #648032 - Flags: review?(ted.mielczarek) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/5b8f3e2803cf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.