Closed Bug 977699 Opened 6 years ago Closed 6 years ago

Move the few remaining mochitests to manifests

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(1 file)

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
Blocks: 920185
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
Attachment #8383170 - Flags: review?(jmaher) → review+
(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.
if they are not used by anything, will they just be in the source tree and not the tests.zip?
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.
These are looking good on try:
https://tbpl.mozilla.org/?tree=Try&rev=ba2a7fc59025 (the orange is from bug 948801).
could we land this patch?  we are so close!
https://hg.mozilla.org/mozilla-central/rev/dfe7b9b49fc8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.