460515 - Remove assumption that xpcshell etc in same directory as app executable

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[cmtalbert]

Mochitests currently make the assumption that xpcshell and other utilities are in the same directory as your application executable.  However on XULRunner projects like Fennec, you often get this directory structure:
<objdir>/dist/bin/:
application.ini
<executable>
components/
chrome/
defaults/
xulrunner/

And inside that xulrunner/ directory is all the normal goodies that Mochitest needs like xpcshell, certutil etc.

While this is pretty standard, I don't think it's set in stone either. So, I advocate for another parameter on runtests.py an app-path (thanks Gavin) argument so that mochitest can look there for other necessary applications and libraries.

The value of app-path should default this way:
* If no parameters set: it should default to be the standard path of the standard firefox installation (current behavior of the tool)
* If a specific parameter is called out using --app and no --app-path specified, then it should default to the containing directory of the executable in --app. (current behavior of the tool)
* Otherwise, app-path is specified, and that value is used instead of the current defaults for this value.

Comment 1

[(not currently active) Ted Mielczarek]

We'll need this if we're ever going to run tests on a packaged build as well.

Comment 2

[Heather Arthur [:harth]]

I looked into this yesterday and overriding the bin-path and certutil path is straightforward. The main problem I found was changing the server base path:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/server.js#131

"/" on the server is mapped to "_tests/testing/mochitest" appended to the install directory. Of course if you move the _tests directory out of the build, this path is bogus. I found no way to get the current working directory instead. If someone could figure this out I think we could get this working.

Comment 3

[(not currently active) Ted Mielczarek]

We could add a --test-path parameter to runtests.py, and pass that test path to xpcshell/server.js via an environment variable. If the env var is unset it can default to what it currently does.

Comment 4

[Heather Arthur [:harth]]

Attachment: patch with flags to override path to binaries and certs

This patch adds three flags to runtests.py. After taking the mochitest directory out of the build, these flags must be specified:

--appname=<absolute path to application executable>
--test-dir=<absolute path to mochitest directory>
--bin-path=<path to binaries>
--certs-path=<path to certificates>

Looking for people to try this on other platforms (I tested it on Ubuntu, both in the build and out of the build). Also, looking for input on the flag names and descriptions. Finally, the test-dir flag is just specifying the directory the script is running from, maybe this should just be a boolean flag instead of making the user type in the absolute path. This is a lot of arguments, maybe it could be bundled in some way.

Comment 5

[(not currently active) Ted Mielczarek]

I think I reinvented your patch while working on bug 421611. I'll take a look at it and see if I can merge the two. As it turns out, I can't keep xpcshell in a different directory for testing packaged builds, as it looks in the same directory as the executable for the components/ dir. RE: comment #2, I'm testing a patch in bug 470914 that adds a __LOCATION__ variable to the global object for files loaded with "xpcshell -f file" (like we currently do for JS components). This should enable server.js to find its current directory (and thus the test path) without any trickery.

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: my WIP

Here's what I've been working on. I have yet to look at your previous patch, but I'll do so before I finish. This patch relies on the patch in bug 470914 to be able to find the test path correctly based on the location of server.js.

Comment 7

[Heather Arthur [:harth]]

That's fine. I think whatever packaging you're doing could be easily ported for Fennec/Xulrunner apps (and easily transfered to the mobile devices), so assuming that the binaries, etc are in one directory is perfectly reasonable.

Comment 8

[(not currently active) Ted Mielczarek]

I'm coordinating with Joel to make sure that my end result is usable for mobile as well, so it should work out well.

Comment 9

[(not currently active) Ted Mielczarek]

After speaking with bsmedberg, we could fix the xpcshell issue I mentioned in comment 5. I filed bug 470971 about it.

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: works for me

Heather: yeah, my patch wound up being eerily similar to yours. :) I'm able to use this to successfully run mochitest from outside my objdir. There are still some caveats, but this is getting closer.

Comment 11

[Heather Arthur [:harth]]

maybe, but I like yours better :-p . Hopefully no mochitests themselves depend on this.

Comment 12

[(not currently active) Ted Mielczarek]

Attachment: add --xre-path

This adds a --xre-path option. After Joel's testing, I realized that my previous approach was broken for the mobile case, since they have separate app and xulrunner dirs. Using this with a mobile build, you'd pass --xre-path=/path/to/xulrunner.

Comment 13

[(not currently active) Ted Mielczarek]

Attachment: cleaned up and updated

Ok, this changes a few things:
1) makes --xre-path default to the directory containing the app binary (so we don't even have to pass it in the non-xulrunner case)
2) Allow relative paths for all the parameters that take filenames
3) Adds an --extra-profile-file parameter, which you can pass multiple times, which lets you specify extra random files to put in the Mochitest profile. This is useful for getting the test plugin into the testing profile.

I've tested this on Windows/Mac/Linux and it works on all three.

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: fix one problem

Oops, Joel tested and noted that I broke --xre-path. Simple fix.

Comment 15

[(not currently active) Ted Mielczarek]

Attachment: fix another problem

Apparently my previous patch broke running without --appname. (Oops.)

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 359737 [details] [diff] [review]
fix another problem

Seems okay, but change getfullpath to getFullPath, camelCaps style.  I wish we didn't have to pass so many parameters here to be able to do this... :-(

Comment 17

[(not currently active) Ted Mielczarek]

Yeah, I had considered an approach whereby we stuck all the necessary bits right in the harness dir when packaging, so the script could look at the default locations from automation.py, and fall back to looking directly in the harness dir. Do you think that's worth pursuing? (Even if so, I don't think having these parameters is bad, since it makes the script's dependencies visible.)

Comment 18

[Jeff Walden [:Waldo]]

Eh...probably not; this is mostly about automated situations like tinderboxen, not really for manual use.

Comment 19

[(not currently active) Ted Mielczarek]

Pushed to m-c (the second time, since the first time broke because of bug 476919):
http://hg.mozilla.org/mozilla-central/rev/2e50c07f49a1

Comment 20

[Serge Gautherie (:sgautherie)]

1.9.1:
This is needed for the context diff of bug 480077.
Note that bug 460515 landed before this bug on branch :-/

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #20)
> Note that bug 460515 landed before this bug on branch :-/

Bug 468913 !

Comment 22

[(not currently active) Ted Mielczarek]

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/415a87eb0e4a