977699 - Move the few remaining mochitests to manifests

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

This gets everything but one chrome mochitest moved into a manifest. This patch has a few quirks, notably I decided to just use the existing make conditionals to conditionally use manifests instead of adding entries to mozinfo. This seemed reasonable given that the conditionals here cover one or two tests each.

A few of the things under testing/mochitest will wind up in slightly different places, I don't know if this will cause issues. I've pushed this patch (+ my other outstanding patches) to try:
https://tbpl.mozilla.org/?tree=Try&rev=ba2a7fc59025

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: Move the few remaining mochitests to manifests

Comment 2

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

Comment on attachment 8383170 [details] [diff] [review]
Move the few remaining mochitests to manifests

Review of attachment 8383170 [details] [diff] [review]:
-----------------------------------------------------------------

a few comments/questions.  I really don't like the idea of using conditions in moz.build to filter out manifests, likewise the trend of creating >1 manifest/directory.  I would like to have a followup bug to investigate a better solution to this via mozinfo + manifestdestiny.  I know we are making 10 steps forward and I don't want to block on it.

::: content/base/test/moz.build
@@ +29,5 @@
> +# (see Bug 774939). App permission checks are also disabled on
> +# anything but B2G (Bug 900707).
> +if CONFIG['MOZ_CHILD_PERMISSIONS']:
> +    MOCHITEST_MANIFESTS += [
> +        'mochitest-child-permissions.ini',

this doesn't do much to aid in our quest to see what tests are enabled/disabled.

::: testing/mochitest/moz.build
@@ +20,5 @@
> +    'dynamic/mochitest.ini',
> +    'MochiKit/mochitest.ini',
> +    'static/mochitest.ini',
> +    'tests/MochiKit-1.4.2/MochiKit/mochitest.ini',
> +    'tests/MochiKit-1.4.2/tests/mochitest.ini',

do these get installed to the proper location?

::: testing/mochitest/static/Makefile.in
@@ -4,5 @@
> -# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> -
> -STATIC_FILES = test.template.txt \
> -		xhtml.template.txt \
> -		xul.template.txt \

I don't see the *.template.txt added to a .ini file

Comment 3

[(not currently active) Ted Mielczarek]

(In reply to Joel Maher (:jmaher) from comment #2)
> a few comments/questions.  I really don't like the idea of using conditions
> in moz.build to filter out manifests, likewise the trend of creating >1
> manifest/directory.  I would like to have a followup bug to investigate a
> better solution to this via mozinfo + manifestdestiny.  I know we are making
> 10 steps forward and I don't want to block on it.

I agree, I'm not wild about the way I implemented that. I don't think it's completely terrible though, since this is disabling tests by *feature*, not by platform. That is, these are tests for features that only make sense if the feature is enabled. I'm open to discussion about it though. The alternative is to stick every one of those conditionals into mozinfo, which is not super appealing, but not technically difficult.

> ::: testing/mochitest/moz.build
> @@ +20,5 @@
> > +    'dynamic/mochitest.ini',
> > +    'MochiKit/mochitest.ini',
> > +    'static/mochitest.ini',
> > +    'tests/MochiKit-1.4.2/MochiKit/mochitest.ini',
> > +    'tests/MochiKit-1.4.2/tests/mochitest.ini',
> 
> do these get installed to the proper location?

The first three do (you'll note the manifests use absolute paths in support-files, which puts files in the right places). The last two go to a slightly different location, but I don't think that matters since they're not referenced from anywhere else. (I'm not even sure why those tests are in this directory, honestly.)

> ::: testing/mochitest/static/Makefile.in
> @@ -4,5 @@
> > -# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > -
> > -STATIC_FILES = test.template.txt \
> > -		xhtml.template.txt \
> > -		xul.template.txt \
> 
> I don't see the *.template.txt added to a .ini file

Yeah, they're not actually used by anything, so I left them out.

Comment 4

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

if they are not used by anything, will they just be in the source tree and not the tests.zip?

Comment 5

[(not currently active) Ted Mielczarek]

Yes. I think that's fine. Their only use is with the gen_template.pl script, and they should work fine from there. There's no point in copying them around and into the test package.

Comment 6

[(not currently active) Ted Mielczarek]

These are looking good on try:
https://tbpl.mozilla.org/?tree=Try&rev=ba2a7fc59025 (the orange is from bug 948801).

Comment 7

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

could we land this patch?  we are so close!

Comment 8

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe7b9b49fc8

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dfe7b9b49fc8