482085 - fix all xpcshell tests to not reference files from the srcdir

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[(not currently active) Ted Mielczarek]

In order to support running tests from packaged builds, we'll need to modify any tests that currently reference files from the source directory. Currently there are two functions that the harness provides for this:
 do_import_script(topsrcdirRelativePath) 
 do_get_file(topsrcdirRelativePath)

It'd be nice to provide similar replacements that work from the test directory, I suppose. Alternately, we could force tests to roll their own using __LOCATION__.

We'll also have to make sure that all necessary files get copied to the objdir. I think currently only .js files in the XPCSHELL_TESTS directories get copied to the objdir.

Comment 1

[(not currently active) Ted Mielczarek]

do_import_script is going to be a pain, since most tests use it to load httpd.js:
http://mxr.mozilla.org/mozilla-central/search?string=do_import_script

I propose that we just add a do_import_httpd_js(), and have that particular script in a location that's known to the harness. The other few instances of do_import_script in the tree could be fixed to reference a local file. I was thinking that I could fix the harness to set the cwd of the xpcshell process to the test directory, so that load("somefile.js") would work, assuming somefile.js was next to the test file.

do_get_file might be even more of a pain, as it looks like we have tests that use it to get an nsIFile for a directory, and then serve that directory via httpd.js. We'll have to see if we can make the build system copy/symlink the entire contents of the XPCSHELL_TESTS dir and have it work properly.

Comment 2

[(not currently active) Ted Mielczarek]

Working on a patch for this, I'm just going to build it on top of the patch from bug 482084.

Comment 3

[(not currently active) Ted Mielczarek]

Waldo points out that a (smarter?) way to fix this would be to make the tests get copied to _tests/xpcshell/$(relativesrcdir)/, then you could load things from the root of _tests/xpcshell.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: fix all the tests

Ok this patch does the following:
1) Copies all files in the XPCSHELL_TESTS dirs into the test dir, not just the test_*.js files
2) Makes the test harness run each test with the cwd=the test dir
3) Removes do_import_script, as you can now simply use load() to load relative pathnames
4) Changes do_get_file to take pathnames relative to the test dir
5) Adds a do_load_httpd_js() function that simply loads that file without worrying about where it's kept.
6) Changes a whole ton of tests, given the above changes

It turns out that the vast majority of tests really just want to access files in their own test dir, so most of this patch is changing things like:
do_get_file("foo/bar/test/unit/file.txt")
to:
do_get_file("file.txt")

There's also a few file moves. Some for where data files were not stored underneath the unit/ dir, and at least one case (extensionmanager) where there were dirs under the unit/ dir that weren't actually needed for the tests, since the Makefile had a special rule to package them into XPIs and stick those in the test dir.

r?bsmedberg on the build bits

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 366566 [details] [diff] [review]
fix all the tests

r? waldo on the harness bits

Comment 6

[(not currently active) Ted Mielczarek]

This isn't working on Windows yet because it relies on nsinstall being able to copy directories successfully, which works fine with the in-tree nsinstall, but not the moztools one.

Comment 7

[Benjamin Smedberg]

Comment on attachment 366566 [details] [diff] [review]
fix all the tests

A couple places you have directoryservice.get("CurWorkD"), but that seems like a lot of code. I'd prefer do_get_file('') or even a new function do_get_curdir()

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: fix all the tests, rev 2

bsmedberg, could you just give this a once-over again? I changed the rules.mk bits slightly to use nsinstall.py on Windows.

I also added a do_get_cwd() and fixed all the tests to use that when they want the current working directory.

Comment 9

[(not currently active) Ted Mielczarek]

FWIW, bugzilla's interdiff between the two patches looks reasonable.

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: fix all the tests, rev 2.1

Slight update to fix a new test.

Comment 11

[Benjamin Smedberg]

Comment on attachment 367857 [details] [diff] [review]
fix all the tests, rev 2.1

reviewed via interdiff, I hope it showed me the important changes!

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 367857 [details] [diff] [review]
fix all the tests, rev 2.1

>diff --git a/config/rules.mk b/config/rules.mk
>--- a/config/rules.mk
>+++ b/config/rules.mk
>@@ -131,9 +131,15 @@
> testxpcobjdir = $(DEPTH)/_tests/xpcshell
> 
> # Test file installation
>+ifdef NSINSTALL_BIN
>+# nsinstall in moztools can't recursively copy directories, so use nsinstall.py
>+TEST_INSTALLER = $(PYTHON) $(topsrcdir)/config/nsinstall.py
>+else
>+TEST_INSTALLER = $(INSTALL)
>+endif
> 
> define _INSTALL_TESTS
>-$(INSTALL) $(wildcard $(srcdir)/$(dir)/*.js) $(testxpcobjdir)/$(MODULE)/$(dir)
>+$(TEST_INSTALLER) $(wildcard $(srcdir)/$(dir)/*) $(testxpcobjdir)/$(MODULE)/$(dir)
> 
> endef # do not remove the blank line!
> 

>diff --git a/js/src/config/rules.mk b/js/src/config/rules.mk
>--- a/js/src/config/rules.mk
>+++ b/js/src/config/rules.mk
>@@ -131,9 +131,15 @@
> testxpcobjdir = $(DEPTH)/_tests/xpcshell
> 
> # Test file installation
>+ifdef NSINSTALL_BIN
>+# nsinstall in moztools can't recursively copy directories, so use nsinstall.py
>+TEST_INSTALLER = $(PYTHON) $(topsrcdir)/config/nsinstall.py
>+else
>+TEST_INSTALLER = $(INSTALL)
>+endif
> 
> define _INSTALL_TESTS
>-$(INSTALL) $(wildcard $(srcdir)/$(dir)/*.js) $(testxpcobjdir)/$(MODULE)/$(dir)
>+$(TEST_INSTALLER) $(wildcard $(srcdir)/$(dir)/*) $(testxpcobjdir)/$(MODULE)/$(dir)
> 
> endef # do not remove the blank line!
> 

This seems unrelated and unintentional.


>diff --git a/testing/xpcshell/head.js b/testing/xpcshell/head.js
>--- a/testing/xpcshell/head.js
>+++ b/testing/xpcshell/head.js
>@@ -171,53 +171,40 @@
>     _do_quit();
> }
> 
>-function do_import_script(topsrcdirRelativePath) {
>-  var scriptPath = environment["TOPSRCDIR"];
>-  if (scriptPath.charAt(scriptPath.length - 1) != "/")
>-    scriptPath += "/";
>-  scriptPath += topsrcdirRelativePath;
>+function do_get_file(path, allowNonexistent) {
>+  try {
>+    let lf = Components.classes["@mozilla.org/file/directory_service;1"]
>+      .getService(Components.interfaces.nsIProperties)
>+      .get("CurWorkD", Components.interfaces.nsILocalFile);
> 
>-  load(scriptPath);
>+    let bits = path.split("/");
>+    for (let i=0; i<bits.length; i++) {
>+      if (bits[i]) {
>+        if (bits[i] == "..")
>+          lf = lf.parent;
>+        else
>+          lf.append(bits[i]);
>+      }
>+    }

Need spaces around = and < here.


>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py

>+  # we assume that httpd.js lives in components/ relative to xpcshell
>+  httpdJSPath = os.path.join(os.path.dirname(xpcshell), "components", "httpd.js").replace("\\", "/");
>+

>-  testharnessdir = os.path.dirname(os.path.abspath(sys.argv[0]))
>-  headfiles = ['-f', os.path.join(testharnessdir, 'head.js')]
>+  headfiles = ['-f', os.path.join(testharnessdir, 'head.js'),
>+               '-e', 'function do_load_httpd_js() {load("%s");}' % httpdJSPath]

I think I want to claw my eyes out now.

Comment 13

[(not currently active) Ted Mielczarek]

(In reply to comment #12)
> This seems unrelated and unintentional.

No, it's necessary and intentional. :) I changed from installing just the test files to the objdir to installing *all files and subdirs in the test dir*, such that we can then tar the whole test dir up for packaging. That's what necessitated the nsinstall.py usage, as the moztools nsinstall.exe doesn't support recursively copying directories.


> I think I want to claw my eyes out now.

Yeah, I'm not wild about that, honestly. Do you have a better suggestion? We can hide the ugliness in head.js and pass it via an environment variable or something. I don't really care, as long as the usage in individual tests is simple.

Comment 14

[Jeff Walden [:Waldo]]

Oh, I should have seen what the rules.mk change was supposed to do.

I still think explicitly listing every file to be copied into an xpcshell parallel tree of test files is the best solution, and then you could just use the normal process of |load("relative/path/to/httpd.js")| with no special helper.

Comment 15

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/a17552ca8f97

Comment 16

[(not currently active) Ted Mielczarek]

(In reply to comment #14)
> I still think explicitly listing every file to be copied into an xpcshell
> parallel tree of test files is the best solution, and then you could just use
> the normal process of |load("relative/path/to/httpd.js")| with no special
> helper.

I'm not a fan of this, since from my experience of fixing every single xpcshell test that used do_import_script / do_get_file, it's clear that the vast majority of tests just want to load files/scripts from the test directory, so it seems silly to require people to specify the full srcdir path in the common case.

Comment 17

[(not currently active) Ted Mielczarek]

I've updated the existing docs (in the URL), but that document needed a lot of editing. I did the best I could, but it could use another pass. Asking for a once-over by Sheppy on it.

Comment 18

[(not currently active) Ted Mielczarek]

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/80c428f2d628