901990 - Install test files through manifests, support manifests for mochitests

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Gregory Szorc [:gps]]

I believe we want to take this opportunity to make mochitests use manifests. Otherwise, we'll do all this work only to have to convert to manifests shortly thereafter.

Comment 2

[Gregory Szorc [:gps]]

I'm at the A*Team work week and will attempt to crank something out in the next few hours.

In a gist, we will be creating separate .ini manifest files for each mochitest suite flavor. e.g. chrome.ini, browserchrome.ini, etc. These manifests will be similar to xpcshell.ini manifests. They'll define *all* test files, including exclusion conditions. They will also define support test files (things living in the same directory or child directories) that are not tests themselves but required to run tests.

In moz.build, we will introduce new variables that define manifest file paths. e.g. MOCHITEST_MANIFESTS, BROWSER_CHROME_MANIFESTS, etc. At moz.build read time, we will open these manifests and extract relevant data and pass it to the backend for consumption.

We will also eventually support defining "global" test support files (e.g. the files in the test runners themselves) in moz.build files. This will enable more efficient installation of everything under _tests and will enable better packaging logic. This work is likely outside the scope of this bug. Bug 899889 would be a good place for this work.

Conversion of test declaration from Makefile.in to manifests will be incremental as it would be too overwhelming to convert everything at once. If a moz.build file contains one of the new *MANIFESTS variables, we will disallow the old MOCHITEST_* variables from existing in the Makefile.in.

As part of conversion, it's likely we'll need to add exclusion/filtering conditions to mozinfo.json. We'll do this as-needed during the process of conversion.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Install test files through manifests

And here's my initial stab at it. This patch is effectively supporting
installing xpcshell test files via manifests (bug 781311). It started as
adding support for new mochitest variables in moz.build files. But as I
was implementing the code for dealing with manifests and as I realized
xpcshell manifests are essentially what mochitests manifests will be, I
decided to just lump xpcshell manifests in while this code was being
touched. If nothing else, it demonstrates some of the funkiness we'll
need to support.

As I was hooking up xpcshell tests, I found a funky one-off.
toolkit/mozapps/extensions/test/Makefile.in copies the xpcshell test
directory in whole and modifies the xpcshell manifest to include an
extra "head" JS file. AFAICT the only reason this hackiness exists is
because the Makefile build rules didn't have a good way of handling
multiple xpcshell manifest files in a single directory. We support that
now, so as part of this patch I removed that hackiness. I made a copy of
xpcshell.ini as xpcshell-unpack.ini and added the extra "head" entry to
it. Furthermore, I added some annotations in the manifest to say that
each manifest file should be kept in sync with the other. If there is
divergence, things will break. This feels a bit hacky to me. Surely
there is a better way to do this? Perhaps we can "merge" manifest files
in the manifest parser, in the moz.build processing step? Maybe we
dynamically generate a new manifest file during moz.build processing?
Lots of options here.

I also removed xpccheck.py from the build rules. I can get away with
this because we now require all files to be annotated in the manifest
files. If someone adds a file to the directory, it won't get packaged
nor executed. Unless it is a support file, it should be obvious from
local runs that things aren't working. Missing support files will get
detected on packaged runs, obviously.

Speaking of support files, that is the one part not yet implemented. We
will need to annotate support files in manifests or in moz.build files.
By "support files" I mean things like
toolkit/components/places/tests/favicons/expected-favicon-big16.ico.png.
If you diff the _tests/xpcshell directory from before and after this
patch, we have about 240 of these files (at least in my build) that will
need annotated. I suppose we could ignore annotating them temporarily
and perform directory scanning. But that seems annoying. I'd rather just
do it right.

I don't know how we wish to annotate support files in manifests. jmaher
put up a strawman at
https://people.mozilla.com/~jmaher/toolkit_passwdmgr_manifest.patch. We
can argue about names, but the concept of adding a new entry with
filenames of support files works for me!

This patch needs a lot of love in the tests department before it can
land. But before that happens, I'd like some f+'s and agreements on the
implementation strategy.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 787348 [details] [diff] [review]
Install test files through manifests

Flagging people who have ideas about build system and manifest foo.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Gregory Szorc [:gps] from comment #3)
> As I was hooking up xpcshell tests, I found a funky one-off.
> toolkit/mozapps/extensions/test/Makefile.in copies the xpcshell test
> directory in whole and modifies the xpcshell manifest to include an
> extra "head" JS file. AFAICT the only reason this hackiness exists is
> because the Makefile build rules didn't have a good way of handling
> multiple xpcshell manifest files in a single directory. We support that
> now, so as part of this patch I removed that hackiness. I made a copy of
> xpcshell.ini as xpcshell-unpack.ini and added the extra "head" entry to
> it. Furthermore, I added some annotations in the manifest to say that
> each manifest file should be kept in sync with the other. If there is
> divergence, things will break. This feels a bit hacky to me. Surely
> there is a better way to do this? Perhaps we can "merge" manifest files
> in the manifest parser, in the moz.build processing step? Maybe we
> dynamically generate a new manifest file during moz.build processing?
> Lots of options here.

Doesn't this format support [include:...]?

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 787348 [details] [diff] [review]
Install test files through manifests

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +145,5 @@
> +        # described inside the instance.
> +        #
> +        # Keys are variable prefixes and values are tuples describing how these
> +        # manifests should be handled:
> +        #    (flavor, install_prefix, strict)

It's non-obvious what "strict" means

@@ +180,5 @@
> +                        manifest_path)
> +
> +                    # If a manifest is marked as a duplicate of another, we
> +                    # verify both manifests have the same content. If not, they
> +                    # are out of sync and they should be updated.

I'm not sure I like this

@@ +219,5 @@
> +                    for f in m.tests[0].get('no-install-files', '').split():
> +                        try:
> +                            del obj.installs[os.path.join(manifest_dir, f)]
> +                        except KeyError:
> +                            pass

Do we want to catch this? It seems like an authoring error.

::: toolkit/mozapps/extensions/test/Makefile.in
@@ -27,5 @@
>  		done \
>  	fi
> -	cd $(TESTROOT)/xpcshell/ && $(TAR) -cPf - . | (cd $(TESTROOT)/xpcshell-unpack && $(TAR) -xPf - )
> -	sed s/head_addons.js/head_addons.js\ head_unpack.js/ $(TESTROOT)/xpcshell-unpack/xpcshell.ini > $(TESTROOT)/xpcshell-unpack/xpcshell.in_
> -	mv $(TESTROOT)/xpcshell-unpack/xpcshell.in_ $(TESTROOT)/xpcshell-unpack/xpcshell.ini

Ugh.

Comment 7

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

Comment on attachment 787348 [details] [diff] [review]
Install test files through manifests

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

while I am not familiar with the mozbuild portion of this code, removing the xpcshell check stuff is a safe thing to do now.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +379,5 @@
> +    'MOCHITEST_TEST_MANIFESTS': (StrictOrderingOnAppendList, list, [],
> +        """List of manifest files defining mochitest tests."""),
> +
> +    'MOCHITEST_CHROME_TEST_MANIFESTS': (StrictOrderingOnAppendList, list, [],
> +        """List of manifest files defining mochitest chrome tests."""),

a nice list, we should fix xpcshell to be XPCSHELL_TESTS_MANIFESTS to be more uniform :)

Comment 8

[Gregory Szorc [:gps]]

Ping on f? requests. I'd like to get support for all this checked into the tree ASAP so people can start the migration of config data to manifests so no-op builds can be faster.

Comment 9

[Jeff Hammel]

(In reply to Gregory Szorc [:gps] from comment #8)
> Ping on f? requests. I'd like to get support for all this checked into the
> tree ASAP so people can start the migration of config data to manifests so
> no-op builds can be faster.

Sorry, was going to give it another pass, but I guess expediency is probably the best option.  Will post hereafter.

Comment 10

[Jeff Hammel]

Comment on attachment 787348 [details] [diff] [review]
Install test files through manifests

> This feels a bit hacky to me. Surely there is a better way to do
> this? Perhaps we can "merge" manifest files in the manifest parser,
> in the moz.build processing step? Maybe we dynamically generate a
> new manifest file during moz.build processing? Lots of options
> here.

ManifestParser already has the capability of reading multiple manifests:

https://github.com/mozilla/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py#L359

And there is [include:] as Ms2ger points out in comment 5 .

Perhaps you can define "merge" here better?  I'd be more than happy to
fix this in manifestparser.py

> I also removed xpccheck.py from the build rules.

Cool.  This means bug 899156 may be killed when this lands.

----

+        self._install_manifests = dict(
+            tests=InstallManifest(),
+        )

A bit weird to split this across three lines IMHO, but whatever

+        # Keys are variable prefixes and values are tuples describing
how these
+        # manifests should be handled:
+        #    (flavor, install_prefix, strict)
+        test_manifests = dict(
+            A11Y_TEST=('a11y', 'testing/mochitest/a11y', True),
+            BROWSER_CHROME_TEST=('browser-chrome',
'testing/mochitest/browser',
+                True),
+            MOCHITEST_TEST=('mochitest', 'testing/mochitest/tests',
True),
+            MOCHITEST_CHROME_TEST=('chrome',
'testing/mochitest/chrome', True),
+            XPCSHELL_TESTS=('xpcshell', 'xpcshell', False),
+        )

Why not namedtuples?  I'd actually be inclined to go with classes
here, as I can imagine this wanting functionality.

+ from manifestparser import TestManifest as InputTestManifest

Strange name choice; why?

+ # TODO add mozinfo into config or somewhere else.

Please file on landing

+                manifest_dir = os.path.dirname(path)
+                manifest_reldir = os.path.dirname(os.path.relpath(path,
+                    self.config.topsrcdir))

Why not os.path.relpath(manifest_dir, self.config.topsrcdir)?

+                    m = InputTestManifest(strict=strict)
+                    m.read(path)

Can also do: m = InputTestManifest(manifests=(path,), strict=strict)

+                    obj = TestManifest(sandbox)
+                    obj.flavor = flavor
+                    obj.manifest = m
+                    obj.manifest_relpath =
os.path.join(obj.relativedir,
+                        manifest_path)

Any reason not to pass these in to the constructor?

> @@ +180,5 @@
> > +                        manifest_path)
> > +
> > +                    # If a manifest is marked as a duplicate of another, we
> > +                    # verify both manifests have the same content. If not, they
> > +                    # are out of sync and they should be updated.
> I'm not sure I like this

Nor I; can we kill this?


Overall, awesome :)  Would love being r?ed for the follow up

Comment 11

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

Comment on attachment 787348 [details] [diff] [review]
Install test files through manifests

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

I think most of this patch falls outside my domain knowledge, but it looks good from what I can see.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +41,5 @@
> +        mozinfo_path = os.path.join(config.topobjdir, 'mozinfo.json')
> +        if os.path.exists(mozinfo_path):
> +            self.mozinfo = json.load(open(mozinfo_path, 'rt'))
> +        else:
> +            self.mozinfo = {}

Are we able to import mozinfo from this context? If so we should just be able to use mozinfo.find_and_update_from_json(), then mozinfo.info later on. If mozbuild is not available at this stage, you can pass in config.topobjdir.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +376,5 @@
> +    'BROWSER_CHROME_TEST_MANIFESTS': (StrictOrderingOnAppendList, list, [],
> +        """List of manifest files defining browser chrome tests."""),
> +
> +    'MOCHITEST_TEST_MANIFESTS': (StrictOrderingOnAppendList, list, [],
> +        """List of manifest files defining mochitest tests."""),

So is the plan to annotate these manifests with skip-if b2g/android for tests that shouldn't be run there?

Comment 12

[Joshua Cranmer [:jcranmer]]

Comment on attachment 787348 [details] [diff] [review]
Install test files through manifests

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ -370,5 @@
>          backend_file.write('PROGRAM = %s\n' % program)
>  
> -    def _process_xpcshell_manifests(self, obj, backend_file, namespace=""):
> -        manifest = obj.xpcshell_manifests
> -        backend_file.write('XPCSHELL_TESTS += %s\n' % os.path.dirname(manifest))

Drive-by comment:

By not adding XPCSHELL_TESTS here, you're breaking subdirectory xpcshell-tests/check-one/check-interactive targets. I think that mach has this functionality in it (it definitely has the first piece), but if you break it, you probably ought to fix the makefile targets.

That said, I'd rather you don't break this for the time being, since mach and comm-central don't get along when it comes to xpcshell-tests (basically, it picks the wrong objdir), and I don't know a clean way to fix it. I expect that building c-c under m-c will end up landing in the 27 timeframe.

Comment 13

[Masatoshi Kimura [:emk]]

Wow, test file changes will no longer be detected on Windows? I will have to run "pymake ... some magic" to update test files every time? I'd appreciate if you fix the broken build system before breaking it further, especially when you will not notice even if you broke it because it is not reproducible on you system.

Comment 14

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

:emk, I am not understanding your concerns?  Can you explain what won't work on windows and what is already broken?

Comment 15

[Masatoshi Kimura [:emk]]

See bug 909508.

Comment 16

[Gregory Szorc [:gps]]

Attachment: Install test files through manifests

I think this patch gets us pretty close in terms of technical
implementation.

The dupe manifest comparing has been removed and I made the extensions
xpcshell test use [include].

I've added support for support files via a "support-files" directive in
xpcshell.ini. It consists of a series of filenames. Because some tests
have dozens of support files, I've added support for wildcards using
mozpack's file finder. As you'll see in the patch, I've used it in a few
places.

This patch still needs love w.r.t. make xpcshell targets and some test
coverage. I think the implementation in emitter.py and recursivemake.py
should be pretty stable going forward. I'd appreciate confirmation of
that assumption.

I expect a handful of test failures due to configurations I haven't yet
tested (I developed and tested with linux desktop).

https://tbpl.mozilla.org/?tree=Try&rev=106ef9fe75f0

Comment 17

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

Comment on attachment 806421 [details] [diff] [review]
Install test files through manifests

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

lots of nits/questions.  Mostly what would be needed are docs to outline how to use the new keywords in the manifests.

::: browser/components/places/tests/unit/xpcshell.ini
@@ +4,2 @@
>  firefox-appdir = browser
> +support-files = bookmarks.glue.html bookmarks.glue.json corruptDB.sqlite distribution.ini

why is this on the same line?  most other files have the support files on a separate line for each file

::: browser/components/sessionstore/test/unit/xpcshell.ini
@@ +2,5 @@
>  head = head.js
>  tail =
>  firefox-appdir = browser
> +support-files =
> +  data/sessionstore_valid.js

why is this on a new line?  what type of parser understands this format?

::: chrome/test/unit/xpcshell.ini
@@ +1,5 @@
>  [DEFAULT]
>  head = head_crtestutils.js
> +tail =
> +support-files =
> +  data/**

why the **?  I am unfamiliar with this syntax?  where are the docs for it?

::: python/mozbuild/mozbuild/frontend/data.py
@@ +285,5 @@
> +        'manifest_relpath',
> +
> +        # If this manifest is a duplicate of another one, this is the
> +        # manifestparser.TestManifest of the other one.
> +        'dupe_manifest',

in general I don't like this, although I am not sure if there is a better solution.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +211,5 @@
> +                True),
> +            MOCHITEST_TEST=('mochitest', 'testing/mochitest/tests', True),
> +            MOCHITEST_CHROME_TEST=('chrome', 'testing/mochitest/chrome', True),
> +            XPCSHELL_TESTS=('xpcshell', 'xpcshell', True),
> +        )

why do we need strict, so far all are True

@@ +235,5 @@
> +                    % path)
> +
> +            obj = TestManifest(sandbox, path, m, flavor=flavor,
> +                relpath=mozpath.join(manifest_reldir, mozpath.basename(path)),
> +                dupe_manifest=m.tests[0].get('dupe-manifest', False))

this assume a boolean value, maybe just a random string will evaluate to true, how do we ensure the end user who adds 'dupe_manifest = false' will get the right behavior?

@@ +237,5 @@
> +            obj = TestManifest(sandbox, path, m, flavor=flavor,
> +                relpath=mozpath.join(manifest_reldir, mozpath.basename(path)),
> +                dupe_manifest=m.tests[0].get('dupe-manifest', False))
> +
> +            filtered = m.active_tests(**self.mozinfo)

we only install filtered tests, could there be a conflict here for builders generating test packages?

@@ +265,5 @@
> +            # While we could feed everything through the finder, we don't
> +            # because we want explicitly listed files that no longer exist
> +            # to raise an error.
> +            for pattern in m.tests[0].get('support-files', '').split():
> +                if '*' in pattern:

we have ** in all our definition, I would like a more detailed list of possible wildcards or regex style syntax we could use.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +440,5 @@
> +    'MOCHITEST_TEST_MANIFESTS': (StrictOrderingOnAppendList, list, [],
> +        """List of manifest files defining mochitest tests."""),
> +
> +    'MOCHITEST_CHROME_TEST_MANIFESTS': (StrictOrderingOnAppendList, list, [],
> +        """List of manifest files defining mochitest chrome tests."""),

awesome!

::: toolkit/components/contentprefs/tests/unit_cps2/xpcshell.ini
@@ +1,4 @@
>  [DEFAULT]
>  head = head.js
>  tail =
> +support-files = AsyncRunner.jsm

this isn't on a new line, different from the other files.

::: toolkit/components/places/tests/queries/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_queries.js
> +tail =

do we need 'support-files = ' even if there is no files?

::: toolkit/devtools/sourcemap/tests/unit/xpcshell.ini
@@ +1,4 @@
>  [DEFAULT]
>  head = head_sourcemap.js
>  tail =
> +support-files = 

nit: trailing whitespace here.

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell-unpack.ini
@@ +4,2 @@
>  firefox-appdir = browser
> +dupe-manifest = 1

where is dupe-manifest defined at? no need for support-files ?

::: toolkit/mozapps/update/test/unit/xpcshell.ini
@@ +1,4 @@
>  [DEFAULT]
>  head = head_update.js
> +tail =
> +autogenerated-files = head_update.js

where is autogenerated-files defined at and documented?

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 806421 [details] [diff] [review]
Install test files through manifests

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

Sorry for the long delay. Overall this looks good, I just have some nitpicking and some suggestions. I'd go as far as to turn this into an r+ if you're otherwise happy with the patch.

::: browser/components/places/tests/unit/xpcshell.ini
@@ +4,2 @@
>  firefox-appdir = browser
> +support-files = bookmarks.glue.html bookmarks.glue.json corruptDB.sqlite distribution.ini

You're inconsistent with your formatting in these manifests.

::: browser/components/sessionstore/test/unit/xpcshell.ini
@@ +2,5 @@
>  head = head.js
>  tail =
>  firefox-appdir = browser
> +support-files =
> +  data/sessionstore_valid.js

This is a valid way to write an ini file? (The format always confuses me.) It seems like single entries should just go on the same line.

::: chrome/test/unit/xpcshell.ini
@@ +1,5 @@
>  [DEFAULT]
>  head = head_crtestutils.js
> +tail =
> +support-files =
> +  data/**

What's with the double star here?

::: config/makefiles/xpcshell.mk
@@ +21,4 @@
>  ###########################################################################
>  # Execute all tests in the $(XPCSHELL_TESTS) directories.
>  # See also testsuite-targets.mk 'xpcshell-tests' target for global execution.
>  xpcshell-tests:

We should totally ditch these targets, even if we don't remove the top-level xpcshell-tests target yet.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +14,5 @@
>  
>  import mozpack.path as mozpath
>  
> +from mozpack.files import FileFinder
> +from manifestparser import TestManifest as InputTestManifest

I wonder if it'd be less confusing to just refer to it as manifestparser.TestManifest?

@@ +58,5 @@
> +        mozinfo_path = os.path.join(config.topobjdir, 'mozinfo.json')
> +        if os.path.exists(mozinfo_path):
> +            self.mozinfo = json.load(open(mozinfo_path, 'rt'))
> +        else:
> +            self.mozinfo = {}

It would be nice to actually use the mozinfo module here, but that probably has a chicken-and-egg problem.

@@ +204,5 @@
> +        #
> +        #    (flavor, install_prefix, strict)
> +        #
> +        # strict determines whether to parse the manifest in strict mode.
> +        test_manifests = dict(

Any reason you didn't write this as a dict literal? That seems more Pythonic.

@@ +242,5 @@
> +            out_dir = mozpath.join(install_prefix, manifest_reldir)
> +
> +            finder = FileFinder(base=manifest_dir, find_executables=False)
> +
> +            for test in filtered:

You should add support for only doing this for some harnesses. xpcshell can handle disabled tests just fine, so they should get installed and skipped by the harness. Once we add that to Mochitest we can just make that the default behavior.

@@ +259,5 @@
> +
> +                        obj.installs[full] = mozpath.join(out_dir, f)
> +
> +            # Pull in support files. Support files are files that aren't
> +            # tests but are required to support them.

Any reason not to merge support files with head/tail files?

@@ +289,5 @@
> +            # part of the build or shouldn't be installed for some
> +            # reason. Here, we prune those files from the install set.
> +            # FUTURE we should be able to detect autogenerated files from
> +            # other build metadata. Once we do that, we can get rid of this.
> +            for f in m.tests[0].get('autogenerated-files', '').split():

"autogenerated-files" seems a bit much, it's not like there's "manually-generated" files. I'd probably just call it "generated-files".

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +437,5 @@
> +    'BROWSER_CHROME_TEST_MANIFESTS': (StrictOrderingOnAppendList, list, [],
> +        """List of manifest files defining browser chrome tests."""),
> +
> +    'MOCHITEST_TEST_MANIFESTS': (StrictOrderingOnAppendList, list, [],
> +        """List of manifest files defining mochitest tests."""),

I'd probably just say "mochitests".

Comment 19

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

Comment on attachment 806421 [details] [diff] [review]
Install test files through manifests

not sure why ted flagged me for f?, I think we had a slight bugzilla mixup.

Comment 20

[Gregory Szorc [:gps]]

(In reply to Joel Maher (:jmaher) from comment #17)
> lots of nits/questions.  Mostly what would be needed are docs to outline how
> to use the new keywords in the manifests.

I'm establishing in-tree build system docs in bug 917988. The next patch will include some docs for the manifest format.
 
> ::: browser/components/places/tests/unit/xpcshell.ini
> @@ +4,2 @@
> >  firefox-appdir = browser
> > +support-files = bookmarks.glue.html bookmarks.glue.json corruptDB.sqlite distribution.ini
> 
> why is this on the same line?  most other files have the support files on a
> separate line for each file

Because it was one of the first files I converted and it was before I realized you could put things on multiple lines :) I'll change it.

> ::: browser/components/sessionstore/test/unit/xpcshell.ini
> @@ +2,5 @@
> >  head = head.js
> >  tail =
> >  firefox-appdir = browser
> > +support-files =
> > +  data/sessionstore_valid.js
> 
> why is this on a new line?  what type of parser understands this format?

Consistency. manifestdestiny surprisingly supports this.

> ::: chrome/test/unit/xpcshell.ini
> @@ +1,5 @@
> >  [DEFAULT]
> >  head = head_crtestutils.js
> > +tail =
> > +support-files =
> > +  data/**
> 
> why the **?  I am unfamiliar with this syntax?  where are the docs for it?

mozpack.files.FileFinder supports this. * is a wildcard for current directory. ** descends into child directories. I'll document it in the next patch.
 
> ::: python/mozbuild/mozbuild/frontend/data.py
> @@ +285,5 @@
> > +        'manifest_relpath',
> > +
> > +        # If this manifest is a duplicate of another one, this is the
> > +        # manifestparser.TestManifest of the other one.
> > +        'dupe_manifest',
> 
> in general I don't like this, although I am not sure if there is a better
> solution.

That sums up my feeling on it too. Strictness or extra metadata annotation: pick one.

Although, I suppose if manfiestdestiny exposed which files were [include:]'d and which files came from which files, I reckon we'd be able to do this without a user-level annotation. Followup bug territory, IMO.

> @@ +237,5 @@
> > +            obj = TestManifest(sandbox, path, m, flavor=flavor,
> > +                relpath=mozpath.join(manifest_reldir, mozpath.basename(path)),
> > +                dupe_manifest=m.tests[0].get('dupe-manifest', False))
> > +
> > +            filtered = m.active_tests(**self.mozinfo)
> 
> we only install filtered tests, could there be a conflict here for builders
> generating test packages?

I don't follow. Can you explain in more detail?

> do we need 'support-files = ' even if there is no files?

No.

Comment 21

[Gregory Szorc [:gps]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #18)
> ::: config/makefiles/xpcshell.mk
> @@ +21,4 @@
> >  ###########################################################################
> >  # Execute all tests in the $(XPCSHELL_TESTS) directories.
> >  # See also testsuite-targets.mk 'xpcshell-tests' target for global execution.
> >  xpcshell-tests:
> 
> We should totally ditch these targets, even if we don't remove the top-level
> xpcshell-tests target yet.

Happily.

FWIW, the survey I sent out a few weeks ago revealed that people use mach for xpcshell tests at a 2:1 margin. I don't recall any specific complaints about mach for xpcshell tests however.

If we remove these targets, we need to be on top of our game to deliver fixes to people when this patch lands. Otherwise, there will be lots of unhappy people.

> @@ +58,5 @@
> > +        mozinfo_path = os.path.join(config.topobjdir, 'mozinfo.json')
> > +        if os.path.exists(mozinfo_path):
> > +            self.mozinfo = json.load(open(mozinfo_path, 'rt'))
> > +        else:
> > +            self.mozinfo = {}
> 
> It would be nice to actually use the mozinfo module here, but that probably
> has a chicken-and-egg problem.

There shouldn't be a chicken-and-egg problem. However, I'm not sure which API to use.

mozinfo handling is generally pretty crummy. Can we just delay attempting to make it better into a followup bug?

> @@ +204,5 @@
> > +        #
> > +        #    (flavor, install_prefix, strict)
> > +        #
> > +        # strict determines whether to parse the manifest in strict mode.
> > +        test_manifests = dict(
> 
> Any reason you didn't write this as a dict literal? That seems more Pythonic.

I like the dict(**kwargs) interface for creating large dicts because IMO it looks prettier since you don't need to quote keys.
 
> @@ +242,5 @@
> > +            out_dir = mozpath.join(install_prefix, manifest_reldir)
> > +
> > +            finder = FileFinder(base=manifest_dir, find_executables=False)
> > +
> > +            for test in filtered:
> 
> You should add support for only doing this for some harnesses. xpcshell can
> handle disabled tests just fine, so they should get installed and skipped by
> the harness. Once we add that to Mochitest we can just make that the default
> behavior.

Will do.

> @@ +259,5 @@
> > +
> > +                        obj.installs[full] = mozpath.join(out_dir, f)
> > +
> > +            # Pull in support files. Support files are files that aren't
> > +            # tests but are required to support them.
> 
> Any reason not to merge support files with head/tail files?

Yes. We don't want head and tail to support the globbing syntax. I'll write a comment and possibly inline the support-files iteration because there is some redundancy here.

> 
> @@ +289,5 @@
> > +            # part of the build or shouldn't be installed for some
> > +            # reason. Here, we prune those files from the install set.
> > +            # FUTURE we should be able to detect autogenerated files from
> > +            # other build metadata. Once we do that, we can get rid of this.
> > +            for f in m.tests[0].get('autogenerated-files', '').split():
> 
> "autogenerated-files" seems a bit much, it's not like there's
> "manually-generated" files. I'd probably just call it "generated-files".

Sure.

> ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
> @@ +437,5 @@
> > +    'BROWSER_CHROME_TEST_MANIFESTS': (StrictOrderingOnAppendList, list, [],
> > +        """List of manifest files defining browser chrome tests."""),
> > +
> > +    'MOCHITEST_TEST_MANIFESTS': (StrictOrderingOnAppendList, list, [],
> > +        """List of manifest files defining mochitest tests."""),
> 
> I'd probably just say "mochitests".

?

Comment 22

[Gregory Szorc [:gps]]

Attachment: Install test files through manifests

I incorporated most of the points of feedback. Notably missing is
sanitizing the support-files format in all the xpcshell.ini. I'll do
that later.

I still need to add some tests.

https://tbpl.mozilla.org/?tree=Try&rev=f7a356189f0b

Comment 23

[Gregory Szorc [:gps]]

Attachment: Part 2: Upgrade xpcshell manifests

This patch converts all the xpcshell.ini manifests to use support-files
and generated-files.

The hardest part of this patch is dealing with the extensions tests,
which were refactored to use [include].

Comment 24

[Gregory Szorc [:gps]]

Attachment: Part 1: Integrate test manifests with build config

I incorporated moar feedback. I wrote some tests. I tweaked how the
files were written in the backend a bit (we now write out a master .ini
manifest for each test flavor encountered). There is still a bit more we
could do (such as writing out a single, machine-readable file describing
all test files and their flavors), but I'm inclined to push those to
other patches or bugs.

I'd push this to try, but this patch relies on patches just pushed to
inbound and inbound is a hot mess right now, so the results wouldn't be
reliable.

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 807538 [details] [diff] [review]
Part 2: Upgrade xpcshell manifests

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

I didn't review every single file, but I spot-checked some.

::: toolkit/components/jsdownloads/test/unit/xpcshell.ini
@@ +2,5 @@
>  head = head.js
>  tail =
> +support-files =
> +  common_test_Download.js
> +# tail.js should quite possibly be in the tail key.

File a followup bug on this?

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini
@@ +1,1 @@
> +# The file is shared between the two main xpcshell manifest files.

nit: can you generate this patch starting from a "hg cp xpcshell.ini xpcshell-shared.ini" for this file?

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
@@ +4,2 @@
>  firefox-appdir = browser
> +dupe-manifest =

I think it'd be nice to put "= true" here, even if the argument is ignored.

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 807567 [details] [diff] [review]
Part 1: Integrate test manifests with build config

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

::: build/docs/test_manifests.rst
@@ +130,5 @@
> +`the source <https://hg.mozilla.org/mozilla-central/file/default/testing/mozbase/manifestdestiny/manifestparser/manifestparser.py>`_.
> +
> +.. todo::
> +
> +   Document manifest filter language.

File a bug on this and assign it to me? I wrote the expression parser.

::: config/makefiles/xpcshell.mk
@@ -1,2 @@
> -# -*- makefile -*-
> -# vim:set ts=8 sw=8 sts=8 noet:

Whee! Should post to dev.platform about this. The top-level "make xpcshell-test" target will still work for now, so I don't think this is a great loss.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +347,5 @@
>          self._update_from_avoid_write(backend_deps.close())
>          self.summary.managed_count += 1
>  
> +        # Make the master test manifest files.
> +        for flavor, manifests in self._test_manifests.items():

Fantastic!

@@ +356,5 @@
> +                flavor)
> +
> +        # The xpcshell.ini file goes to an additional location for historical
> +        # reasons. Use of this file should eventually be replaced with the
> +        # above location.

File a followup bug?

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +54,5 @@
>          self.populate_logger()
>  
>          self.config = config
>  
> +        # TODO add mozinfo into config or somewhere else.

Followup bug?

@@ +288,5 @@
> +                        else:
> +                            full = mozpath.normpath(mozpath.join(manifest_dir,
> +                                pattern))
> +                            # Only install paths in our directory. This
> +                            # rule is somewhat arbitrary and could be lifted.

Make sure this is documented.

Comment 27

[Gregory Szorc [:gps]]

https://tbpl.mozilla.org/?tree=Try&rev=42d9c5d164d2

Comment 28

[(not currently active) Ted Mielczarek]

Comment on attachment 807567 [details] [diff] [review]
Part 1: Integrate test manifests with build config

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +349,5 @@
>  
> +        # Make the master test manifest files.
> +        for flavor, manifests in self._test_manifests.items():
> +            self._write_master_test_manifest(os.path.join(
> +                self.environment.topobjdir, '_tests', '%s.ini' % flavor),

nit: after using this for my patch in bug 919635, I think this actually wants to be:
os.path.join(self.environment.topobjdir, '_tests', flavor, '%s.ini' % flavor))

Right now you wind up with $objdir/_tests/$flavor.ini, and the tests under $objdir/_tests/$flavor/$relativesrcdir, but the paths in the ini are all $relativesrcdir, so the includes wouldn't work directly, you'd have to shuffle the ini file around.

Comment 29

[(not currently active) Ted Mielczarek]

(I guess that's not actually flavor I want there, but install_prefix from emitter.py.)

Comment 30

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbafc6a77569
https://hg.mozilla.org/integration/mozilla-inbound/rev/a75639eeca26

Applied feedback before checkin and without explicit review since they were minor and since I got verbal OK from ted over IRC.

Will file the followup bugs...

Comment 32

[Gregory Szorc [:gps]]

We broke PGO on Linux due to very silly reason. Pushed a patch with IRC review from glandium.

It's hacky. We need a better story around the logic between the two builds during a PGO build.

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

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bbafc6a77569
https://hg.mozilla.org/mozilla-central/rev/a75639eeca26
https://hg.mozilla.org/mozilla-central/rev/b51710e0e485