Closed Bug 810617 Opened 7 years ago Closed 7 years ago

Pass browser app directory to xpcshell test harness

Categories

(Core :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [metro-it1])

Attachments

(1 file, 8 obsolete files)

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.
Attached patch fix v.1 (obsolete) — Splinter Review
Assignee: nobody → jmathies
Summary: Elm test failures caused by services/common/tests/unit/head_helpers.js → Pass browser app directory to xpcshell test harness
Duplicate of this bug: 810299
Duplicate of this bug: 810307
Comment on attachment 681460 [details] [diff] [review]
fix v.1

Thoughts on this approach?
Attachment #681460 - Flags: feedback?(mh+mozilla)
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.
Attachment #681460 - Flags: feedback?(mh+mozilla)
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.
Attached patch wip v.1 (obsolete) — Splinter Review
Attachment #681460 - Attachment is obsolete: true
Attached patch appdir (obsolete) — Splinter Review
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.
Attached patch wip v.2 (obsolete) — Splinter Review
Attachment #681745 - Attachment is obsolete: true
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.
Attachment #681928 - Flags: feedback?(mh+mozilla)
(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
Attached patch appDirKey patch (obsolete) — Splinter Review
This appears to work. I'll put together a full patch with all the changes.
Attachment #682026 - Attachment description: appKey patch → appDirKey patch
Attached patch wip v.3 (obsolete) — Splinter Review
Attachment #681928 - Attachment is obsolete: true
Attachment #682026 - Attachment is obsolete: true
Attachment #681928 - Flags: feedback?(mh+mozilla)
Attached patch wip v.3 (obsolete) — Splinter Review
- ini file updates as well
Attachment #682028 - Attachment is obsolete: true
Attached patch make check fix (obsolete) — Splinter Review
fix for some make check errors on elm, MOZ_APP_NAME shouldn't have been marked as required in writemozinfo.py.
Attachment #681922 - Attachment is obsolete: true
Whiteboard: metro-it1
Whiteboard: metro-it1 → [metro-it1]
Attached patch wip v.4Splinter Review
I'll post the final xpcshell.ini patch once we get past updater test failure problems.
Attachment #682033 - Attachment is obsolete: true
Attachment #682122 - Attachment is obsolete: true
Attachment #682480 - Flags: review?(mh+mozilla)
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?
Attachment #682480 - Flags: review?(ted)
Attachment #682480 - Flags: review?(mh+mozilla)
Attachment #682480 - Flags: feedback+
Duplicate of this bug: 815011
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.
Attachment #682480 - Flags: review?(ted) → review+
> 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.
(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.
https://hg.mozilla.org/mozilla-central/rev/6def19c0c235
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
See Also: → 1261461
You need to log in before you can comment on or make changes to this bug.