Add instrumentation manifests to moz.build

RESOLVED FIXED in mozilla35

Status

RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

unspecified
mozilla35
All
Android

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 8491848 [details] [diff] [review]
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.
Attachment #8491848 - Flags: review?(mshal)
<bikeshedding>no-install is a weird name for this feature</bikeshedding>
(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 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.
Attachment #8491848 - Flags: review?(mshal) → review+
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?
(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.
Created attachment 8492418 [details] [diff] [review]
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.
Attachment #8491848 - Attachment is obsolete: true
Attachment #8492418 - Flags: review?(ted)
(Assignee)

Updated

5 years ago
Summary: Add no-install flag to test manifests to allow not install test paths → Add instrumentation manifests to moz.build
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.
Attachment #8492418 - Flags: review?(ted) → review+
Assignee: nobody → nalexander
(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 11

5 years ago
https://hg.mozilla.org/mozilla-central/rev/5698141fb2b8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.