fix all xpcshell tests to not reference files from the srcdir

RESOLVED FIXED

Status

Testing
XPCShell Harness
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

({dev-doc-needed, fixed1.9.1})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

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.
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.
Working on a patch for this, I'm just going to build it on top of the patch from bug 482084.
Depends on: 482084
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.
Created attachment 366566 [details] [diff] [review]
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
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
Attachment #366566 - Flags: review?(benjamin)
Attachment #366566 - Flags: review?(jwalden+bmo)
Comment on attachment 366566 [details] [diff] [review]
fix all the tests

r? waldo on the harness bits
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

9 years ago
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()
Attachment #366566 - Flags: review?(benjamin) → review+
Created attachment 367206 [details] [diff] [review]
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.
Attachment #366566 - Attachment is obsolete: true
Attachment #367206 - Flags: review?(jwalden+bmo)
Attachment #367206 - Flags: review?(benjamin)
Attachment #366566 - Flags: review?(jwalden+bmo)
FWIW, bugzilla's interdiff between the two patches looks reasonable.
Created attachment 367857 [details] [diff] [review]
fix all the tests, rev 2.1

Slight update to fix a new test.
Attachment #367206 - Attachment is obsolete: true
Attachment #367857 - Flags: review?(jwalden+bmo)
Attachment #367857 - Flags: review?(benjamin)
Attachment #367206 - Flags: review?(jwalden+bmo)
Attachment #367206 - Flags: review?(benjamin)

Comment 11

9 years ago
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!
Attachment #367857 - Flags: review?(benjamin) → review+

Updated

9 years ago
Attachment #367857 - Flags: review?(jwalden+bmo) → review+
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.
(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.
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.
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/a17552ca8f97
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(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.
Depends on: 482768
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.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.