869635 - Eliminate the master xpcshell manifest

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Joshua Cranmer [:jcranmer]]

Attachment: First cut

xpcshell tests are now in moz.build, so we now have one tool that already knows about every test in the entire testsuite. Instead of requiring developers to list every directory in xpcshell.ini and hoping the best, let's just emit that file as needed.

Comment 1

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

You're losing the verification that all test_* files are in the manifest next to it, no?

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to :Ms2ger from comment #1)
> You're losing the verification that all test_* files are in the manifest
> next to it, no?

That would at least avoid hacks like this:
http://mxr.mozilla.org/comm-central/source/mail/test/xpcshell.ini#20

Comment 3

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

why don't we just add a build flag or something to detect if lightning is being built?  With so many ways to disable tests, it becomes a nightmare determining what should be running and what shouldn't be as you change stuff.  A manifest doesn't help us unless it is the source of truth.  Removing the check to ensure all tests are in the manifest and vice versa will only allow us to get sloppy.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

There is MOZ_CALENDAR defined, but afaik this has not been hooked up to something accessible from xpcshell.ini. Anyway, I think Joshua's intent for this bug wasn't related to Lightning but on a more general note.

Comment 5

[Joshua Cranmer [:jcranmer]]

(In reply to :Ms2ger from comment #1)
> You're losing the verification that all test_* files are in the manifest
> next to it, no?

Hmm, you're right, I may have been a bit too greedy here. The check I want to lose is that all xpcshell.ini files are located in mozilla/testing/xpcshell/xpcshell.ini [since that file won't exist anymore].

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 746614 [details] [diff] [review]
First cut

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

The build system changes look in order. Some minimal tests would be nice, of course.

I agree with previous comments that removing the checks in xpccheck.py is going too far.

You should consider parsing xpcshell.ini files during moz.build traversal, as part of emitter.py. I view test manifests as extensions to moz.build files and part of the larger build metadata ecosystem. We will eventually need to do this anyway so build backends know which test files need to be installed. Feel free to parse the manifests and add the parsed metadata to the XpcshellManifests class instances.

I do wonder when the best time to run the file auditing is. moz.build traversal seems like a candidate. However, file additions or removals may not incur moz.build traversal. It might be easiest to leave the check as part of the build.

Comment 7

[Joshua Cranmer [:jcranmer]]

Attachment: Generate the master manifest from moz.build

There are other features that could potentially come from having this information located in the master repository, such as being able to run xpcshell-tests for all subdirectories of the current directory [which broke when we first moved to manifests].

The xpccheck has been restored (I forgot that said file handled both the "is this manifest registered" and the "are these tests registered" checks), but I have excised the first part. The xpcshell.ini file removal is liable to cause lots of spurious patch conflicts.

As for moving the manifest parsing and checking to the mozbuild generation, I'm hesitant to do that for the moment. People stuff lots of things in xpcshell tests other than what the manifests say (do a grep for _tests/xpcshell), so they're not a good record of what is needed to stuff in packaged tests yet. Running xpccheck at configure time feels like the wrong thing to do, since the leaf xpcshell.ini files don't get added to the re-configure. Ultimately, my main goal here is to enable comm-central to have working tests in a cc-rework world, and this patch is needed to make it work.

I don't plan on landing this until the comm-central config.status issue is fixed.

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Relevant changes to comm-central

With the master manifest now automatically generated, we no longer need our xpcshell tests hack. Almost. Lightning tests don't work if they're packaged (see bug 680203), so we retain a clever trick to disable Lightning tests only for packaged builds (hint: our buildbots use all-test-dirs.list, not xpcshell.ini).

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 754527 [details] [diff] [review]
Generate the master manifest from moz.build

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

You'll get r+ as soon as there is a minimal unit test that verifies a master manifest is correctly produced.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +238,5 @@
>              backend_deps.write('%s:\n' % path)
>  
>          self._update_from_avoid_write(backend_deps.close())
>          self.summary.managed_count += 1
> + 

Nit: trailing whitespace

::: testing/xpcshell/xpcshell.ini
@@ -125,5 @@
> -[include:toolkit/components/commandlines/test/unit_win/xpcshell.ini]
> -skip-if = os != "win"
> -
> -[include:toolkit/components/commandlines/test/unit_unix/xpcshell.ini]
> -skip-if = os == "win" || os == "mac" || os == "os2"

These conditionals are somewhat concerning. If these files/directories are already protected by conditionals in moz.build files, we are fine. Otherwise, this could regress behavior. I will audit this set of paths before final review.

Comment 10

[Joshua Cranmer [:jcranmer]]

Attachment: Generate the master manifest from moz.build

Now with tests. The actual deletion of the master manifest is prone to bitrot (since people keep adding new test directories), but I verified that all the conditional stuff was correct when I first looked at it and I don't think anyone's added new conditions since.

Comment 11

[Joshua Cranmer [:jcranmer]]

Comment on attachment 761227 [details] [diff] [review]
Generate the master manifest from moz.build

Requesting review would help..

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 761227 [details] [diff] [review]
Generate the master manifest from moz.build

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

\o/

::: testing/xpcshell/Makefile.in
@@ +48,5 @@
>  # Rules for staging the necessary harness bits for a test package
>  PKG_STAGE = $(DIST)/test-package-stage
>  
>  libs::
> +	$(INSTALL) xpcshell.ini $(DEPTH)/_tests/xpcshell

I wonder if we could eventually generate the target file directly instead of having to install it here. Likely followup fodder.

Comment 13

[Joshua Cranmer [:jcranmer]]

(In reply to Gregory Szorc [:gps] from comment #12)
> ::: testing/xpcshell/Makefile.in
> @@ +48,5 @@
> >  # Rules for staging the necessary harness bits for a test package
> >  PKG_STAGE = $(DIST)/test-package-stage
> >  
> >  libs::
> > +	$(INSTALL) xpcshell.ini $(DEPTH)/_tests/xpcshell
> 
> I wonder if we could eventually generate the target file directly instead of
> having to install it here. Likely followup fodder.

Step 1 of make in the build tree:
default alldep all:: CLOBBER $(topsrcdir)/configure config.status backend.RecursiveMakeBackend.built
[ ... ]
	$(RM) -r _tests

Until we fix our build system to not clobber unconditionally, we can't do this.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 754528 [details] [diff] [review]
Relevant changes to comm-central

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

Looks great, thanks :-)

Comment 15

[Gregory Szorc [:gps]]

(In reply to Joshua Cranmer [:jcranmer] from comment #13)
> Until we fix our build system to not clobber unconditionally, we can't do
> this.

Write the master xpcshell.ini into topobjdir directly and install it into _tests at the appropriate time?

Comment 16

[Joshua Cranmer [:jcranmer]]

Mozilla-central patch pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1006a2fff399

Comm-central will be pushed when m-i merges to m-c.

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1006a2fff399

Comment 18

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/47d08a243bde