Pass browser app directory to xpcshell test harness

RESOLVED FIXED in mozilla20

Status

()

Core
General
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
mozilla20
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [metro-it1])

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 681460 [details] [diff] [review]
fix v.1
Assignee: nobody → jmathies
(Assignee)

Updated

5 years ago
Summary: Elm test failures caused by services/common/tests/unit/head_helpers.js → Pass browser app directory to xpcshell test harness
(Assignee)

Updated

5 years ago
Duplicate of this bug: 810299
(Assignee)

Updated

5 years ago
Duplicate of this bug: 810307
(Assignee)

Comment 10

5 years ago
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)
(Assignee)

Comment 12

5 years ago
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.
(Assignee)

Comment 13

5 years ago
Created attachment 681745 [details] [diff] [review]
wip v.1
Attachment #681460 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 681922 [details] [diff] [review]
appdir

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.
(Assignee)

Comment 15

5 years ago
Created attachment 681928 [details] [diff] [review]
wip v.2
Attachment #681745 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
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
(Assignee)

Comment 18

5 years ago
Created attachment 682026 [details] [diff] [review]
appDirKey patch

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

Updated

5 years ago
Attachment #682026 - Attachment description: appKey patch → appDirKey patch
(Assignee)

Comment 19

5 years ago
Created attachment 682028 [details] [diff] [review]
wip v.3
Attachment #681928 - Attachment is obsolete: true
Attachment #682026 - Attachment is obsolete: true
Attachment #681928 - Flags: feedback?(mh+mozilla)
(Assignee)

Comment 20

5 years ago
Created attachment 682033 [details] [diff] [review]
wip v.3

- ini file updates as well
Attachment #682028 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 682122 [details] [diff] [review]
make check fix

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

Updated

5 years ago
Attachment #681922 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: metro-it1
Whiteboard: metro-it1 → [metro-it1]
(Assignee)

Comment 22

5 years ago
Created attachment 682480 [details] [diff] [review]
wip v.4

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+
(Assignee)

Comment 28

5 years ago
> 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.
(Assignee)

Comment 29

5 years ago
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...
(Assignee)

Comment 31

5 years ago
(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.
(Assignee)

Comment 34

5 years ago
(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.
(Assignee)

Comment 36

5 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Updated

2 years ago
See Also: → bug 1261461
You need to log in before you can comment on or make changes to this bug.