Closed Bug 460515 Opened 15 years ago Closed 14 years ago

Remove assumption that xpcshell etc in same directory as app executable


(Testing :: Mochitest, defect)

Not set


(Not tracked)



(Reporter: cmtalbert, Assigned: ted)



(Keywords: fixed1.9.1)


(1 file, 6 obsolete files)

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:

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 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.
We'll need this if we're ever going to run tests on a packaged build as well.
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:

"/" 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.
We could add a --test-path parameter to, 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.
This patch adds three flags to 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.
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.
Blocks: 421611
Attached patch my WIP (obsolete) — Splinter Review
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.
Depends on: 470914
Assignee: nobody → ted.mielczarek
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.
I'm coordinating with Joel to make sure that my end result is usable for mobile as well, so it should work out well.
After speaking with bsmedberg, we could fix the xpcshell issue I mentioned in comment 5. I filed bug 470971 about it.
Depends on: 470971
Attached patch works for me (obsolete) — Splinter Review
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.
Attachment #350088 - Attachment is obsolete: true
Attachment #354308 - Attachment is obsolete: true
maybe, but I like yours better :-p . Hopefully no mochitests themselves depend on this.
Attached patch add --xre-path (obsolete) — Splinter Review
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.
Attachment #354325 - Attachment is obsolete: true
Attached patch cleaned up and updated (obsolete) — Splinter Review
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.
Attachment #357143 - Attachment is obsolete: true
Attachment #359121 - Flags: review?(jwalden+bmo)
Attached patch fix one problem (obsolete) — Splinter Review
Oops, Joel tested and noted that I broke --xre-path. Simple fix.
Attachment #359121 - Attachment is obsolete: true
Attachment #359299 - Flags: review?(jwalden+bmo)
Attachment #359121 - Flags: review?(jwalden+bmo)
Apparently my previous patch broke running without --appname. (Oops.)
Attachment #359299 - Attachment is obsolete: true
Attachment #359737 - Flags: review?(jwalden+bmo)
Attachment #359299 - Flags: review?(jwalden+bmo)
Attachment #359737 - Flags: review?(jwalden+bmo) → review+
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... :-(
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, 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.)
Eh...probably not; this is mostly about automated situations like tinderboxen, not really for manual use.
Depends on: 476919
Pushed to m-c (the second time, since the first time broke because of bug 476919):
Closed: 14 years ago
Resolution: --- → FIXED
This is needed for the context diff of bug 480077.
Note that bug 460515 landed before this bug on branch :-/
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
(In reply to comment #20)
> Note that bug 460515 landed before this bug on branch :-/

Bug 468913 !
Pushed to 1.9.1:
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
You need to log in before you can comment on or make changes to this bug.