1069648 - Add instrumentation manifests to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

For certain types of tests, the test paths function as identifiers rather than actual file names.  My use case is for Android instrumentation tests, like Robocop tests.  I want to say that each test is a Java file, and I want the harness to complain if a test is missing (see Bug 1069569); but I don't want to actually include the Java files in the test package.

Some of the reasons to use the filenames: we can add INSTRUMENTATION_MANIFESTS to moz.build; we can re-use existing all-tests.json logic for finding tests; and we make autocompletion and run-by-directory possible in some future test harness.

I think the reason to not include Java files is clear.  I reckon there are other suites where the test is a compiled artifact rather than the test file itself (cppunittests?), but you still want to reference the test by the test file name.

I propose a no-install manifest flag to handle this situation.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Add no-install flag to test manifests. r=mshal

This is used to mark a test path as a test identifier.  Such test
identifiers might correspond to compiled tests.  Test paths marked
no-install are not installed to the _tests directory, or bundled into
the test package.

Comment 2

[Mike Hommey [:glandium]]

<bikeshedding>no-install is a weird name for this feature</bikeshedding>

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #2)
> <bikeshedding>no-install is a weird name for this feature</bikeshedding>

I rejected a few longer ones (no-install-test-path).  We want something that makes sense for a single test and as a default in the DEFAULTS section (more common, I expect).  I like "bikeshed magenta" but welcome suggestions :)

Comment 4

[Michael Shal [:mshal]]

Comment on attachment 8491848 [details] [diff] [review]
Add no-install flag to test manifests. r=mshal

This seems fine. Regarding a better name, would something like "package-test = False" work instead of "no-install = True"?

In any case, I don't expect a name change to materially change this patch, so r+ for now. Maybe you can ping glandium to discuss a better name.

Comment 5

[(not currently active) Ted Mielczarek]

Is there a reason you want this as an annotation in the test manifest, and not just hardcoded as the behavior in the moz.build reader? If you forget to include it in a manifest you'll wind up including unnecessary files. If we never want to copy the test files in this type of manifest, why make developer specify that per-manifest?

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> Is there a reason you want this as an annotation in the test manifest, and
> not just hardcoded as the behavior in the moz.build reader? If you forget to
> include it in a manifest you'll wind up including unnecessary files. If we
> never want to copy the test files in this type of manifest, why make
> developer specify that per-manifest?

I actually did this first, and then decided that a per-test, per-manifest flexible thing was more likely to be valuable in future.  If you feel strongly, I can do this instead.

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Add instrumentation manifests to moz.build. r=ted

These manifests are special in that they don't package their test files
into the test package.  Each test listed in an instrumentation manifest
serves as an identifier rather than a file.

ted: here's the approach you preferred.  I folded in my new manifest
type because without a manifest type that wanted the new
functionality, it would be onerous to test.  The actual manifests
landed aren't consumed yet.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8492418 [details] [diff] [review]
Add instrumentation manifests to moz.build. r=ted

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

Kind of a bikeshed comment, but "INSTRUMENTATION_MANIFESTS" seems pretty vague to me. It would be clearer if you tacked ANDROID_ on to the beginning, but I'll leave that up to you. Thanks for writing unit tests for your new emitter behavior.

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py
@@ +705,5 @@
> +        man_dir = mozpath.join(env.topobjdir, '_build_manifests', 'install')
> +        self.assertTrue(os.path.isdir(man_dir))
> +
> +        expected = ['tests']
> +        for e in expected:

I'm not sure why this is a loop...

@@ +722,5 @@
> +            uninstalled_files = [
> +                'instrumentation/./not_packaged.java',
> +            ]
> +            for f in uninstalled_files:
> +                self.assertFalse(f in m)

It would make the test more resilient if you iterated all the files in the InstallManifest and asserted that none of them had a basename that matched not_packaged.java. As written, this test would give a false positive if someone normalized the paths going into the test manifest.

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> Comment on attachment 8492418 [details] [diff] [review]
> Add instrumentation manifests to moz.build. r=ted
> 
> Review of attachment 8492418 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Kind of a bikeshed comment, but "INSTRUMENTATION_MANIFESTS" seems pretty
> vague to me. It would be clearer if you tacked ANDROID_ on to the beginning,
> but I'll leave that up to you. Thanks for writing unit tests for your new
> emitter behavior.
> 
> ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py
> @@ +705,5 @@
> > +        man_dir = mozpath.join(env.topobjdir, '_build_manifests', 'install')
> > +        self.assertTrue(os.path.isdir(man_dir))
> > +
> > +        expected = ['tests']
> > +        for e in expected:
> 
> I'm not sure why this is a loop...

Vestigial.

> @@ +722,5 @@
> > +            uninstalled_files = [
> > +                'instrumentation/./not_packaged.java',
> > +            ]
> > +            for f in uninstalled_files:
> > +                self.assertFalse(f in m)
> 
> It would make the test more resilient if you iterated all the files in the
> InstallManifest and asserted that none of them had a basename that matched
> not_packaged.java. As written, this test would give a false positive if
> someone normalized the paths going into the test manifest.

It would, except there's (currently) no (publicly exposed) way to iterate an InstallManifest.  I'm going to keep this as is for now.

Comment 10

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/5698141fb2b8

Comment 11

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/5698141fb2b8