Provide common location for testing modules

RESOLVED FIXED in mozilla15

Status

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

({dev-doc-complete})

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

7 years ago
It is common for my xpcshell, mochitest, etc tests to make use of shared testing artifacts, like mocks. e.g. I create a mock HTTP server so I can write tests that pretend to hit the real deal. This mock HTTP server is needed by tests in toolkit/ *and* services/.

In the current world, we have to resort to:

* Elevating the test-only code to "production" status and install and package it with the distribution (e.g. dist/bin/modules/).
* Declaring "head" files in xpcshell.ini and using relative file imports.
* Using load() to explicitly load some file (from a location not registered with the URL handling mechanism like resource://)

head xpcshell.ini and load() are effectively equivalent. And, they are hard to use because you deal with cryptic paths like ../../../tests/xpcshell/head_foo.js. Shipping unnecessary test code is just silly for obvious reasons.

I would like to propose the creation of a well-defined and easy-to-use shared repository of test-related artifacts/modules. How this is done exactly, I'm not sure.

From the perspective of JS testing, I would like to support JS modules that can be imported via Components.utils.import() and not via load(). e.g. Cu.import("testing://services/storageserver.js"); What the exact registered URL should be and where that should point to, that's an open question.

I think this bug should include:

* Determining whether to put shared testing artifacts. Perhaps somewhere in objdir/testing (so they don't get included in the distribution). Although, I'm not sure what the requirements are.
* Telling the test harnesses about this location (e.g. hooking up URL handlers)
* Providing easy-to-use build system integration so definition of a variable or two magically results in files being installed to the proper location (e.g. like how files in modules/ magically get installed to the right place).
Assignee

Comment 1

7 years ago
This patch is pretty basic. I'm guessing the contentious part would be the location of the installed testing files. If anyone has a better idea, I'm open to hearing it.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #618716 - Flags: review?(ted.mielczarek)
Attachment #618716 - Flags: review?(gavin.sharp)
Assignee

Comment 2

7 years ago
This registers resource://testing/ in xpcshell tests so it points to objdir/_tests/modules/

Try at http://tbpl.mozilla.org/?tree=Try&rev=557ed98f6419

Not sure how I can add test coverage for this. But, I've verified it works (on OS X at least).
Attachment #618717 - Flags: review?(ted.mielczarek)
Attachment #618717 - Flags: review?(gavin.sharp)
Comment on attachment 618716 [details] [diff] [review]
Add Makefile rules to install testing-only JS modules

This all looks OK to me; I don't think I can provide any value beyond Ted's review.
Attachment #618716 - Flags: review?(gavin.sharp)
Attachment #618717 - Flags: review?(gavin.sharp)
Assignee

Comment 4

7 years ago
This version makes the TESTING_MODULES_DIR argument to xpcshell optional. Before, if it wasn't defined, head.js would crash.

All tests on the first try failed because the build infrastructure isn't adding the new runxpcshelltests.py argument. Once this is deployed, we'll have to modify the build infrastructure to add this new argument. Until that follow-up change is made, any tests that rely on this new functionality won't work. Fortunately, no tests yet rely on this change.

New try at http://tbpl.mozilla.org/?tree=Try&rev=c99bd2a0687e
Attachment #618717 - Attachment is obsolete: true
Attachment #618717 - Flags: review?(ted.mielczarek)
Attachment #618815 - Flags: review?(ted.mielczarek)
Try run for 557ed98f6419 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=557ed98f6419
Results (out of 16 total builds):
    success: 3
    warnings: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-557ed98f6419
Try run for c99bd2a0687e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c99bd2a0687e
Results (out of 16 total builds):
    success: 9
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-c99bd2a0687e
Assignee

Comment 7

7 years ago
I have high hopes this one will pass try (https://tbpl.mozilla.org/?tree=Try&rev=cea63c7c8bbb).
Attachment #618815 - Attachment is obsolete: true
Attachment #618815 - Flags: review?(ted.mielczarek)
Attachment #619804 - Flags: review?(ted.mielczarek)
Try run for f99bb55423fe is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f99bb55423fe
Results (out of 16 total builds):
    exception: 3
    success: 6
    warnings: 4
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-f99bb55423fe
Try run for cea63c7c8bbb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cea63c7c8bbb
Results (out of 16 total builds):
    success: 15
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-cea63c7c8bbb
Comment on attachment 618716 [details] [diff] [review]
Add Makefile rules to install testing-only JS modules

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

::: config/rules.mk
@@ +1590,5 @@
> +ifdef TESTING_JS_MODULES
> +testmodulesdir = $(DEPTH)/_tests/modules/$(TESTING_JS_MODULE_DIR)
> +
> +libs::
> +	$(NSINSTALL) -D $(testmodulesdir)

You should use:
GENERATED_DIRS = $(testmodulesdir)
http://mxr.mozilla.org/mozilla-central/source/config/makefiles/autotargets.mk#23
(that sets up a real dependency)
Attachment #618716 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 619804 [details] [diff] [review]
Part 2: Integrate xpcshell with testing modules, v3

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

r+ with a couple of naming nits.

::: testing/testsuite-targets.mk
@@ +244,5 @@
>  	  --manifest=$(DEPTH)/_tests/xpcshell/xpcshell.ini \
>  	  --build-info-json=$(DEPTH)/mozinfo.json \
>  	  --no-logfiles \
>  	  --tests-root-dir=$(call core_abspath,_tests/xpcshell) \
> +	  --tests-modules-dir=$(call core_abspath,_tests/modules) \

Hoo boy, every time I review something like this I really want bug 744540 fixed!

::: testing/xpcshell/head.js
@@ +302,5 @@
>  }
>  
>  function _execute_test() {
> +  // Map resource://test/ to current working directory and
> +  // resource://testing/ to the shared test modules directory.

That seems confusingly similar. Can we call it resource://testing-common/ or resource://common/ or something like that?

::: testing/xpcshell/runxpcshelltests.py
@@ +232,5 @@
> +        '-n',
> +        '-s',
> +        '-e', 'const _HTTPD_JS_PATH = "%s";' % self.httpdJSPath,
> +        '-e', 'const _HEAD_JS_PATH = "%s";' % self.headJSPath
> +    ]

Nice reformatting. :)

@@ +234,5 @@
> +        '-e', 'const _HTTPD_JS_PATH = "%s";' % self.httpdJSPath,
> +        '-e', 'const _HEAD_JS_PATH = "%s";' % self.headJSPath
> +    ]
> +
> +    if self.testingModulesDir is not None:

I generally just prefer "if self.testingModulesDir:", but maybe the surrounding code says otherwise here?

@@ +900,5 @@
>      self.add_option("--tests-root-dir",
>                      type="string", dest="testsRootDir", default=None,
>                      help="absolute path to directory where all tests are located. this is typically $(objdir)/_tests")
> +    self.add_option("--tests-modules-dir",
> +                    dest="testingModulesDir", default=None,

This option is a bit awkwardly named. Maybe --testing-modules-dir?
Attachment #619804 - Flags: review?(ted.mielczarek) → review+
Assignee

Comment 12

7 years ago
Switch to GENERATED_DIRS made and tested locally.

I also pushed part 2 prematurely. Addressed review nits in new part 3. Oops.

https://hg.mozilla.org/integration/mozilla-inbound/rev/137aabbc13a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b063ba6dd084
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e4e9519516
Target Milestone: --- → mozilla15
You rock, gps! We should get this documented on MDN so that developers will put their test helpers in reusable modules rather than head files...

I also know a couple of places (*cough* no pun intended) that could benefit from a head -> jsm refactoring... but that's for another bug :)
Keywords: dev-doc-needed
Assignee

Comment 14

7 years ago
Yeah, I need to throw something up on MDN. Not sure where though. Here are the choices:

https://developer.mozilla.org/en/Mozilla_automated_testing
https://developer.mozilla.org/en/Running_automated_tests
https://developer.mozilla.org/en/Developing_Tests

The testing documentation just needs a lot of love overall.

FWIW, I threw up a blog post at http://gregoryszorc.com/blog/2012/05/10/better-sharing-of-test-code-in-mozilla-projects/. I'm being hypocritical of my own rule of not using personal blogs as a replacement for MDN :/
Assignee

Updated

7 years ago
Blocks: 744323
Assignee

Updated

7 years ago
No longer blocks: 744323
Assignee

Updated

7 years ago
Blocks: 755339
See Also: → 904812
You need to log in before you can comment on or make changes to this bug.