http://mxr.mozilla.org/mozilla-central/source/services/common/tests/unit/head_helpers.js TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\xpcshell\tests\services\aitc\tests\unit\test_load_modules.js | test failed (with xpcshell return code: 3), see following log: >>>>>>> C:/talos-slave/test/build/xpcshell/tests/services/common/tests/unit/head_helpers.js:5: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import] <<<<<<< https://tbpl.mozilla.org/php/getParsedLog.php?id=16928378&tree=Elm#error35 Followed by about hundred additional services failures with the same missing import.

I'm not sure what to do about these failures. services code lands in the app dir, so xpc shell tests that try to use service code all fail. Maybe services code should land in gre vs app? Seems odd that they end up in the app directory in the first place.

My guess is the elm builds are missing the resource://services-common registration. That can easily be tested by someone with a running elm build.

(In reply to Gregory Szorc [:gps] from comment #2) > My guess is the elm builds are missing the resource://services-common > registration. That can easily be tested by someone with a running elm build. They are defined in the browser, but not for tests. Elm has the current implementation of bug 755724. So looking at the dist/bin, the services code is in browser/modules and metro/modules. I think tests expect this code to be in /modules.

This is the typical case where the xpcshell needs to be run with an additional -a argument pointing at the browser/ directory.

(In reply to Mike Hommey [:glandium] from comment #4) > This is the typical case where the xpcshell needs to be run with an > additional -a argument pointing at the browser/ directory. Ah, great idea. That makes the cleanup of this whole xpcshell test mess much easier.

(In reply to Jim Mathies [:jimm] from comment #5) > (In reply to Mike Hommey [:glandium] from comment #4) > > This is the typical case where the xpcshell needs to be run with an > > additional -a argument pointing at the browser/ directory. > > Ah, great idea. That makes the cleanup of this whole xpcshell test mess much > easier. We already had support for this through the -a option. I landed an experimental fix on elm just to see if it would work and things cleared up quite a bit. I'll work up a real patch for mc this week.

Comment on attachment 681460 [details] [diff] [review] fix v.1 Thoughts on this approach?

Comment on attachment 681460 [details] [diff] [review] fix v.1 Review of attachment 681460 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/testsuite-targets.mk @@ +247,5 @@ > +ifneq ($(NULL),$(MOZ_NEW_PACKAGER)) > + ifeq ($(MOZ_BUILD_APP),browser) > + XPCSHELL_BROWSER_ARGS = --app-path=$(DIST)/bin/$(MOZ_DESKTOP_APP_DIR) > + endif > +endif I see several problems with this: - tinderbox slaves don't use that (sadly) - this makes all xpcshell tests rely on the browser/ app directory, while we could very well have webapprt or metro xpcshell tests (we currently can't, so we don't have any, but i guess as soon as we can, we would like to have some). I think it would make sense to add the app subdirectory to the relevant xpcshell test manifests.

So for the .ini file idea, I was thinking you could have something like: product-appdir = browser or product-appdir = mail where 'product' is the app name and the value is a sub dir off the gre path that gets passed to the test harness. with this, testing webapps / metro on an xpcshell test run using a firefox zip you could have: firefox-appdir = metro or firefox-appdir = webapprt But the buildbot command line doesn't have much of anything we can use to define the product. For example: python -u xpcshell/runxpcshelltests.py --symbols-path=http://ftp...mbols.zip --manifest=xpcshell/tests/all-test-dirs.list firefox/xpcshell.exe or python -u xpcshell/runxpcshelltests.py --symbols-path=http://ftp...mbols.zip --manifest=xpcshell/tests/all-test-dirs.list ./FirefoxNightly.app/Contents/MacOS/xpcshell We could do some string searching on the working directory maybe, or update buildbot so that it hands us something we can use.

The final incarnation of this still needs to be sorted out, but here's a list of all the test locations (thus far) that need the browser dir defined.

Comment on attachment 681928 [details] [diff] [review] wip v.2 Mike, please see comment 12 for some detail. A half baked solution is on elm. This works on mc, but if this landed on the thunderbird repo it would obviously break. I'm not sure how we resolve this. I think the thunderbird build pulls from mc (could be wrong, but I remember having to do this for local builds), so I believe we need a more complete solution. Plus there's still the issue of local builds for projects like xulrunner.

(In reply to Jim Mathies [:jimm] from comment #12) > But the buildbot command line doesn't have much of anything we can use to > define the product. For example: We can fix that. runxpcshelltests.py picks up a bunch of info about the build from the mozinfo.json file (you can find it in the root of your objdir, it defaults to looking for it next to the script which is why it's not in the buildbot commandline): http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#121 It would be very straightforward to stick the product name in there. We write it during configure: http://mxr.mozilla.org/mozilla-central/source/configure.in#8902 with this script: http://mxr.mozilla.org/mozilla-central/source/config/writemozinfo.py

This appears to work. I'll put together a full patch with all the changes.

- ini file updates as well

fix for some make check errors on elm, MOZ_APP_NAME shouldn't have been marked as required in writemozinfo.py.

I'll post the final xpcshell.ini patch once we get past updater test failure problems.

Comment on attachment 682480 [details] [diff] [review] wip v.4 Review of attachment 682480 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, but i'm not familiar with this code enough to make it a r+. ::: config/writemozinfo.py @@ +37,5 @@ > d["os"] = o.lower() > > # Widget toolkit, just pass the value directly through. > d["toolkit"] = env["MOZ_WIDGET_TOOLKIT"] > Can you remove the extra whitespaces while you're here?

appdirs thus far: https://hg.mozilla.org/projects/elm/rev/04d950c6a442 https://hg.mozilla.org/projects/elm/rev/4115b1b19d5d https://hg.mozilla.org/projects/elm/rev/747ddbfae45d (bug 815320)

Comment on attachment 682480 [details] [diff] [review] wip v.4 Review of attachment 682480 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/xpcshell/runxpcshelltests.py @@ +730,5 @@ > + # defined we pass 'grePath/$appDirKey' for the -a parameter of the xpcshell > + # test harness. > + appDirKey = None > + if "appname" in self.mozInfo: > + appDirKey = self.mozInfo["appname"] + "-appdir" So the test manifests would have something like: [test_foo.js] browser-appdir = metro ? @@ +779,5 @@ > > # Check for known-fail tests > expected = test['expected'] == 'pass' > > + # Handle application directory path settings This could use a slightly more informative comment. @@ +780,5 @@ > # Check for known-fail tests > expected = test['expected'] == 'pass' > > + # Handle application directory path settings > + if appDirKey != None and appDirKey in test: The != None test isn't really necessary. @@ +785,5 @@ > + relAppDir = test[appDirKey] > + relAppDir = os.path.join(self.xrePath, relAppDir) > + relAppDir = os.path.normpath(relAppDir) > + if not os.path.isabs(relAppDir): > + relAppDir = os.path.abspath(relAppDir) Inconsistent indentation here, the rest of this file uses 2-space. Also, abspath will call normpath for you, so you can just unconditionally call abspath instead of the 3 lines you have here.

appdirs thus far: https://hg.mozilla.org/projects/elm/rev/04d950c6a442 https://hg.mozilla.org/projects/elm/rev/4115b1b19d5d https://hg.mozilla.org/projects/elm/rev/747ddbfae45d (bug 815320) https://hg.mozilla.org/projects/elm/rev/eccc38673808

> So the test manifests would have something like: > [test_foo.js] > browser-appdir = metro > > ? Correct. Since the failing tests tend to be grouped, we've been putting most of these in the [DEFAULT] section for the most part. > > # Check for known-fail tests > > expected = test['expected'] == 'pass' > > > > + # Handle application directory path settings > > This could use a slightly more informative comment. will update. > > @@ +780,5 @@ > > # Check for known-fail tests > > expected = test['expected'] == 'pass' > > > > + # Handle application directory path settings > > + if appDirKey != None and appDirKey in test: > > The != None test isn't really necessary. Wasn't sure, my python skillz aren't robuts. Could 'test' contain an entry that is equal to None? Doesn't seem to do any harm to leave it in there, FWIW. > > @@ +785,5 @@ > > + relAppDir = test[appDirKey] > > + relAppDir = os.path.join(self.xrePath, relAppDir) > > + relAppDir = os.path.normpath(relAppDir) > > + if not os.path.isabs(relAppDir): > > + relAppDir = os.path.abspath(relAppDir) > > Inconsistent indentation here, the rest of this file uses 2-space. Also, > abspath will call normpath for you, so you can just unconditionally call > abspath instead of the 3 lines you have here. ok, will update.

neither are my typing skills - s/robuts/robust

I'm wondering how well this will work with standalone xulrunner and Firefox-on-xulrunner (or anything-else-on-xulrunner) builds...

(In reply to Mike Hommey [:glandium] from comment #30) > I'm wondering how well this will work with standalone xulrunner and > Firefox-on-xulrunner (or anything-else-on-xulrunner) builds... I'mnot sure about Firefox-on-xulrunner, but for xulrunner apps won't xulrunner-appdir = (some-gre-subdir) suffice?

(In reply to Jim Mathies [:jimm] from comment #31) > (In reply to Mike Hommey [:glandium] from comment #30) > > I'm wondering how well this will work with standalone xulrunner and > > Firefox-on-xulrunner (or anything-else-on-xulrunner) builds... > > I'mnot sure about Firefox-on-xulrunner, but for xulrunner apps won't > > xulrunner-appdir = (some-gre-subdir) > > suffice? Technically, there is no appdir for a xulrunner build. Which is where the problem lies. Some non-browser tests in e.g. toolkit require an appdir... For Firefox-on-xulrunner, it might just work if we move browser files under browser for these builds as well.

If we have tests in toolkit that require the browser appdir that seems pretty wrong and we should fix it in some way.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #33) > If we have tests in toolkit that require the browser appdir that seems > pretty wrong and we should fix it in some way. There are a few in components/places, components/search, mozapps/extensions, and the mozapps/update. I started working on these by touching up individual tests, but the changes started to grow. Most of the changes involve setting prefs initially that aren't present. The updater tests had more serious issues without an app dir.

appdirs thus far: https://hg.mozilla.org/projects/elm/rev/04d950c6a442 https://hg.mozilla.org/projects/elm/rev/4115b1b19d5d https://hg.mozilla.org/projects/elm/rev/747ddbfae45d (bug 815320) https://hg.mozilla.org/projects/elm/rev/eccc38673808 https://hg.mozilla.org/projects/elm/rev/c64e4494b21e That's it!

(In reply to Jim Mathies [:jimm] from comment #35) > appdirs thus far: > > https://hg.mozilla.org/projects/elm/rev/04d950c6a442 > https://hg.mozilla.org/projects/elm/rev/4115b1b19d5d > https://hg.mozilla.org/projects/elm/rev/747ddbfae45d (bug 815320) > https://hg.mozilla.org/projects/elm/rev/eccc38673808 > https://hg.mozilla.org/projects/elm/rev/c64e4494b21e > > That's it! We've been removing the need for some of these on elm in follow ups. So I'm going to land the xpcshell work here on mc and let this bug close out. I'll file a follow up for firefox-appdir entries for mc that blocks bug 755724.