Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Eliminate the master xpcshell manifest

RESOLVED FIXED in mozilla24

Status

Testing
XPCShell Harness
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla24
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 746614 [details] [diff] [review]
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.
Attachment #746614 - Flags: feedback?(gps)
You're losing the verification that all test_* files are in the manifest next to it, no?
(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
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.
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.
(Assignee)

Comment 5

4 years ago
(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

4 years ago
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.
Attachment #746614 - Flags: feedback?(gps) → feedback+
(Assignee)

Updated

4 years ago
Depends on: 870565
(Assignee)

Comment 7

4 years ago
Created attachment 754527 [details] [diff] [review]
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.
Attachment #746614 - Attachment is obsolete: true
Attachment #754527 - Flags: review?(gps)
(Assignee)

Comment 8

4 years ago
Created attachment 754528 [details] [diff] [review]
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).
(Assignee)

Updated

4 years ago
Attachment #754528 - Flags: review?(mbanner)

Comment 9

4 years ago
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.
Attachment #754527 - Flags: review?(gps) → feedback+
(Assignee)

Comment 10

4 years ago
Created attachment 761227 [details] [diff] [review]
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.
Attachment #754527 - Attachment is obsolete: true
(Assignee)

Comment 11

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

Requesting review would help..
Attachment #761227 - Flags: review?(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.
Attachment #761227 - Flags: review?(gps) → review+
(Assignee)

Comment 13

4 years ago
(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 on attachment 754528 [details] [diff] [review]
Relevant changes to comm-central

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

Looks great, thanks :-)
Attachment #754528 - Flags: review?(mbanner) → review+
(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?
(Assignee)

Comment 16

4 years ago
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.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/1006a2fff399
https://hg.mozilla.org/comm-central/rev/47d08a243bde
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla24

Updated

4 years ago
Blocks: 896695
You need to log in before you can comment on or make changes to this bug.