Move package-tests out of Makefile.in

NEW
Unassigned

Status

Testing
General
5 years ago
4 years ago

People

(Reporter: gps, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
We have a number of Makefile.in in the tree that contain targets supporting |make package-tests|. Combined, they effectively constitute a giant shell script. They don't need to be in make files. Where, exactly, I'm not sure. We could probably annotate info in moz.build files. I think I'd also be fine with just moving things into a Python script for round 1.
comm-central needs to add mozmill (and possibly more) to package-tests, and we need to munge a file between its runtime generation and package-tests.
(Reporter)

Comment 2

5 years ago
Created attachment 783554 [details] [diff] [review]
Move package-tests out of make files

Here is the start of what I have in mind. We create some Python module
holding the logic for creating the test archive. It knows what goes in
and where to put it.

If we do things right, we can abstract the underlying archive format
(allowing us to easily write zip files, bz2 files, etc). More
importantly, we can avoid extra file I/O by having Python write directly
into an archive file handle instead of staging all files in a common
directory first. But why stop there: we may even be able to eliminate
some of the file copies into the _tests directory!

Anyway, I'm interested in what others feel about this approach.
(Reporter)

Updated

5 years ago
Assignee: nobody → gps
(Reporter)

Comment 3

5 years ago
Comment on attachment 783554 [details] [diff] [review]
Move package-tests out of make files

Looking for an initial reaction.
Attachment #783554 - Flags: feedback?(ted)
(Reporter)

Updated

5 years ago
Assignee: gps → nobody
Some brief comments on the patch:

I prefer the separation of the packaging into a core packaging step and a packaging step per testsuite. This allows us (in theory, in any case) to make the testsuites that get packaged dependent on which testsuites are actually going to be usefully runnable for a given build configuration. It's not quite so important for mozilla-central, but Thunderbird needs to package mozmill tests and doesn't use mochitests or reftests at all.

Hardcoding specific configuration names for conditional inclusion in the packaged tests also strikes me as a sign that the way the packaging is being driven is suboptimal.
Comment on attachment 783554 [details] [diff] [review]
Move package-tests out of make files

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

::: python/mozbuild/mozbuild/packaging.py
@@ +89,5 @@
> +            yield (f.path, j('certs', p))
> +
> +        # Plugins
> +        finder = FileFinder(j(self.distdir, 'plugins'), find_executables=False)
> +        for plugin in ('Test.plugin', 'SecondTest.plugin'):

This sounds mac-only.

@@ +117,5 @@
> +            yield (f.path, j('mochitest', p))
> +
> +        if substs['MOZ_BUILD_APP'] == 'mobile/android':
> +            yield (j(s, 'mobile/android/base/fennec_ids.txt'),
> +                j('mochitest', 'fennec_ids.txt'))

Overall, this function seems overly complicated for something that could probably be written with a simple preprocessed manifest (which is what i eventually intended to do, btw, using the packager code)
(Reporter)

Comment 6

5 years ago
So it sounds like people want to move things into moz.build and/or packager manifests. That's fine by me!

It sounds like we at least need a distributed definition to facilitate c-c. So, multiple manifest files around the tree with moz.build references to collect them all?
Comment on attachment 783554 [details] [diff] [review]
Move package-tests out of make files

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

I'm not wild about this specific implementation. I think the concept of getting it out of Makefiles is good, but I think I'd just like a manifest and using the packager backend.

::: python/mozbuild/mozbuild/packaging.py
@@ +117,5 @@
> +            yield (f.path, j('mochitest', p))
> +
> +        if substs['MOZ_BUILD_APP'] == 'mobile/android':
> +            yield (j(s, 'mobile/android/base/fennec_ids.txt'),
> +                j('mochitest', 'fennec_ids.txt'))

I have to agree. I feel like we could be a lot more declarative about this. 99% of it is just copying files.
Attachment #783554 - Flags: feedback?(ted) → feedback-
I think bug 976733 is what we want, and we should WONTFIX this.
You need to log in before you can comment on or make changes to this bug.