568733 - patch for mobile specific browser-chrome tests breaks seamonkey

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

in bug 535922, we filter out browser specific tests by doing this:
+ifeq (browser,$(MOZ_BUILD_APP))
 DIRS = \
   browser \
   $(NULL)
+endif

this ends up breaking seaMonkey builds.  It would be better to filter based on something like this:
ifnew (mobile,$(MOZ_BUILD_APP))
DIRS = ...
endif

Comment 1

[Justin Wood (:Callek)]

(In reply to comment #0)
> this ends up breaking seaMonkey builds.  It would be better to filter based on
> something like this:
> ifnew (mobile,$(MOZ_BUILD_APP))
> DIRS = ...
> endif

Just to be clear, we are not "broken" by this, just our test coverage was severely reduced; and We do not want to loose the coverage.

Comment 2

[(not currently active) Ted Mielczarek]

Yeah, I wasn't really happy about the solution there in the first place. :-(

Just filtering out 'mobile' will work, unless Thunderbird (or some other app) decides they want to run tests with the browser-chrome harness, and breaks on the browser-specific tests.

Comment 3

[Robert Kaiser]

(In reply to comment #2)
> Just filtering out 'mobile' will work, unless Thunderbird (or some other app)
> decides they want to run tests with the browser-chrome harness, and breaks on
> the browser-specific tests.

I also thing filtering out 'mobile' from a build system side is probably best. And if Thunderbird or anyone else wants to run browser-chrome, they'd better implement a browser in some way (they can't even run any mochitest harness so far), and even if they would, I think such things are best to be dealt with on a case-by-case basis.

Comment 4

[[e-mail]]

(In reply to comment #1)
> Just to be clear, we are not "broken" by this
Actually we are. See bug 535922 comment #20.

Comment 5

[Justin Wood (:Callek)]

Attachment: v1

This should do it.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 450030 [details] [diff] [review]
v1

>diff --git a/layout/style/test/Makefile.in b/layout/style/test/Makefile.in
>--- a/layout/style/test/Makefile.in
>+++ b/layout/style/test/Makefile.in
>@@ -219,7 +219,7 @@
> 		$(topsrcdir)/layout/reftests/svg/pseudo-classes-02-ref.svg \
> 		$(NULL)
> 
>-ifeq (browser,$(MOZ_BUILD_APP))
>+ifneq (mobile,$(MOZ_BUILD_APP))
> _BROWSER_FILES = \
> 		browser_bug453896.js \
> 		bug453896_iframe.html \

in the original patch there was a media_queries_iframe.html listed in this block.


>diff --git a/toolkit/mozapps/extensions/test/Makefile.in b/toolkit/mozapps/extensions/test/Makefile.in
>--- a/toolkit/mozapps/extensions/test/Makefile.in
>+++ b/toolkit/mozapps/extensions/test/Makefile.in
>@@ -52,7 +52,7 @@
>   xpinstall \
>   $(NULL)
> 
>-ifeq (browser,$(MOZ_BUILD_APP))
>+ifneq (mobile,$(MOZ_BUILD_APP))
>  DIRS += browser
> endif
> 

in bug 570812 we want to pull xpinstall into the ifneq block.  We might as well fix it here since we are fiddling with everything.

Comment 7

[Justin Wood (:Callek)]

(In reply to comment #6)
> (From update of attachment 450030 [details] [diff] [review])
> >diff --git a/layout/style/test/Makefile.in b/layout/style/test/Makefile.in
> >--- a/layout/style/test/Makefile.in
> >+++ b/layout/style/test/Makefile.in
> >@@ -219,7 +219,7 @@
> > 		$(topsrcdir)/layout/reftests/svg/pseudo-classes-02-ref.svg \
> > 		$(NULL)
> > 
> >-ifeq (browser,$(MOZ_BUILD_APP))
> >+ifneq (mobile,$(MOZ_BUILD_APP))
> > _BROWSER_FILES = \
> > 		browser_bug453896.js \
> > 		bug453896_iframe.html \
> 
> in the original patch there was a media_queries_iframe.html listed in this
> block.

I'm not sure why a contextual part of the original patch and a contextual part of this one matters...

> >diff --git a/toolkit/mozapps/extensions/test/Makefile.in b/toolkit/mozapps/extensions/test/Makefile.in
> >--- a/toolkit/mozapps/extensions/test/Makefile.in
> >+++ b/toolkit/mozapps/extensions/test/Makefile.in
> >@@ -52,7 +52,7 @@
> >   xpinstall \
> >   $(NULL)
> > 
> >-ifeq (browser,$(MOZ_BUILD_APP))
> >+ifneq (mobile,$(MOZ_BUILD_APP))
> >  DIRS += browser
> > endif
> > 
> 
> in bug 570812 we want to pull xpinstall into the ifneq block.  We might as well
> fix it here since we are fiddling with everything.

I'd prefer to fix it in the other patch, as *this* is blocking SeaMonkey, but I can get it fixed and pushed together if it is reviewed in time as well.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 450030 [details] [diff] [review]
v1

Still not a great solution, but not any worse than before.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

I still see tests for:
docshell/test/browser
toolkit/mozapps/extensions/test/xpinstall

I did verify my mozconfig had --enable-application=mobile and that the Makefile for docshell/test/Makefile(.in) had the correct modification.

The toolkit issue is bug 570812 and mentioned in comment #6.

Comment 10

[Justin Wood (:Callek)]

Attachment: v2

Porting ted's r+

(In reply to comment #9)
> I still see tests for:
> docshell/test/browser
> toolkit/mozapps/extensions/test/xpinstall
> 
> I did verify my mozconfig had --enable-application=mobile and that the Makefile
> for docshell/test/Makefile(.in) had the correct modification.

Hmm, actually it did NOT have the correct modification. These tests were inadvertently disabled for Firefox too when this first landed, since it is checking for existance of a flag BEFORE its defined in the Makefile.

> The toolkit issue is bug 570812 and mentioned in comment #6.

And I asked a Q there, before c#6 here. And I also requested to allow me to fix that orthogonal to this. (As soon as I get my answer there I'll attach a patch).

Due to the dochsell issue, I'm planning to land this within a few minutes, even before your feedback though.

Comment 11

[Justin Wood (:Callek)]

http://hg.mozilla.org/mozilla-central/rev/1d840cf822e6