Closed Bug 616999 Opened 14 years ago Closed 13 years ago

switch to a manifest format for xpcshell instead of walking directories

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 9 obsolete files)

53.98 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
6.50 KB, patch
ted
: review+
Details | Diff | Splinter Review
46.44 KB, patch
ted
: review+
Details | Diff | Splinter Review
1.25 KB, patch
k0scist
: review+
Details | Diff | Splinter Review
currently with the xpcshell harness, we build at compile time a list of directories to test.  When we run the harness, we read that list of directories and look for head_*.js, tail_*.js and test_*.js by using os.walk in python.  This is reliably and good, but difficult to control what tests run on specific platforms.  Currently we tweak this with the Makefiles which is done during compile time.
Attached patch use manifests for xpcshell (0.1) (obsolete) — Splinter Review
initial patch that is more of a proof of concept rather than something we want to call a final solution.  This does pass on try for all platforms (except OSX Opt builds- must be that universal binary stuff).

This patch generates a ini based manifest format for all-test-dirs.list from the build.  Then during runtime, it reads each directory and creates a .ini file.  Finally it reads the ini files and uses that data to run the tests.  

The next steps are to add .ini files into the source tree and put logic in there to turn on/off tests based on key/values (tags).
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Probably a dupe of bug 563726, but who cares.
Attached patch use manifests for xpcshell (0.9) (obsolete) — Splinter Review
updated patch to run from manifests and double check that we have all tests files in the hardcoded manifests.

passes green on try except for OSX opt (universal build comparison fails)
Attachment #495500 - Attachment is obsolete: true
Attachment #497955 - Flags: feedback?(jhammel)
Comment on attachment 497955 [details] [diff] [review]
use manifests for xpcshell (0.9)

The section names are supposed to be paths relative to the .ini file.
Instead, they are paths relative to some arbitrary point. The
ManifestParser gives a 'path' key which is the absolute path to the
test. Instead, the 'name' (section name) key is used here:

+    for test in hg.tests:
+      name = test['name'].replace('/', os.sep)
+      try:
+        x = ininames.index(os.path.join(dirpath, name))
+      except:
+        failed = True
+        print "!!data in hg manifest only: '" + str(name) + "'"

Also, if the "test['name'].replace('/', os.sep)" needs to be done, it
should be moved to the ManifestParser.  Is there any reason not to
make these changes?

Similarly for 'head' and 'tail' entries.  These are a bit harder,
though, since the ManifestParser doesn't know that these are supposed
to be paths.  Two options I can think of:

 1. The version of manifests.py here provides a variable, %(here)s,
 which is the directory name of the current .ini file that is
 automatically interpolated.  Since then, this sort of interpolation
 has been taken out of trunk:
 http://k0s.org/mozilla/hg/ManifestDestiny/rev/802442d97e2c . I could
 put this back in, if this is generally useful, and we could add more
 variables in if desired.

 2. An integration layer that is xpcshell-specific could do this logic
 instead.

I don't know how either of the above path issues work with the fact
that we run tests out of various places (srcdir, objdir). As best I
can tell, it shouldn't have a negative effect since the manifests will
live alongside the tests in all cases (I think).

I'm also not sure our strategy for updating m-c from more recent
versions of manifests.py.  I know this is a larger issue, but
commenting for posterity's sake.

I don't really understand the logic for buildlist.py....but that's
probably not important.  Maybe worth revisiting when manifestdestiny
becomes more of a reality.

+    sectionre = re.compile('^\[.*\]$')
+    for line in f:
+      if (sectionre.match(line)):
+        hgdata.append(line)

Is there any reason not to use the .ini parser already built in?
::shrug::  Itd be nice to know the intent of this function.  It feels
like something that should/could be moved to manifests.py.

Never use bare `except` clauses when you know what the exception type
is (in this case, ValueError).

Also, its probably easier to test using `in` vs index:

if line not in hgdata:
  failed = True
  print "!!data in generated manifest only: " + str(line)

I would do this function entirely differently, assuming I understand
the intent.  Using ConfigParser, for example (though read_ini could be
used too), I'd do something like this:

hgparser = RawConfigParser()
hgparser.read(os.path.join(os.path.realpath(os.path.dirname(sys.argv[0]))
               'xpcshell.ini'))
genparser = RawConfigParser()
genparser.read(self.manifest  +'.ini')
failed = False
for section in hgparser.sections():
  if section not in genparser.sections():
    print "!!data in hg manifest only: " + section
    failed = True
for section in genparser.sections():
  if section not in hgparser.sections():
    print "!!data in hg manifest only: " + section
    failed = True
...

Though again, i think if the goal is just to diff the sections in .ini
files, this should go -> manifests.py  since it is generic functionality

os.path.abspath(os.path.realpath(os.path.dirname(sys.argv[0])

^ os.path.realpath is an absolute path:

>>> os.path.realpath('foo')
'/home/jhammel/mozilla/src/mozilla-central/foo'

You can probably remove the commented out code too.

It looks like os, os.ignore, and toolkit should go up the chain to
manifests.py.  I'm not sure how and if we decided to do this.  It'd be
nice to move towards something that works across harnesses and maybe
moves towards making automation.py.in just automation.py

Other than that it looks like a good first shot.  I don't have a
strong preference whether these things are dealt with before landing
or as cleanup (though it'd be nice if they weren't forgotten about).
Attachment #497955 - Flags: feedback?(jhammel) → feedback+
It'd be nice if we didn't have to repeat the full path for each file. Has there been any discussion about the format?
(In reply to comment #5)
> It'd be nice if we didn't have to repeat the full path for each file. Has there
> been any discussion about the format?

The intention is to have the test name (the part in the [section]) be a path relative to the manifest.  I believe :jmaher intends to use this syntax before it is landed.
updated patch to use relative filenames and updated manifestparser.py.  Overall, this is cleaner and greener.  Lets get another round of feedback, then I will prepare for official review.
Attachment #497955 - Attachment is obsolete: true
Attachment #511141 - Flags: feedback?(jhammel)
Comment on attachment 511141 [details] [diff] [review]
use manifests for xpcshell (0.99)

>--- /dev/null
>+++ b/build/manifestparser.py
>@@ -0,0 +1,780 @@
>+#!/usr/bin/env python
>+
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+# 
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+# 
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+# 
>+# The Original Code is mozilla.org code.
>+# 
>+# The Initial Developer of the Original Code is
>+# Mozilla.org.

the Mozilla Foundation.
Comment on attachment 511141 [details] [diff] [review]
use manifests for xpcshell (0.99)

+        if inimode:
+           fini.write("[include:%s/xpcshell_gen.ini]\n" % e)

Why xpcshell_gen.ini vs just xpcshell?

-relativesrcdir = services/crypto/components/tests
+relativesrcdir = services/crypto/component/tests

Looking at the source tree, it looks like this just corrects a
hitherto unnoticed typo?

 stage-package:
        $(NSINSTALL) -D $(PKG_STAGE)/xpcshell/tests
        @(cd $(srcdir) && tar $(TAR_CREATE_FLAGS) -
        $(TEST_HARNESS_FILES)) | (cd $(PKG_STAGE)/xpcshell && tar -xf
        -)
+       @(cp $(srcdir)/xpcshell.ini $(PKG_STAGE)/xpcshell/tests)

Since you have to have this rule to copy to the subdirectory, is there
any reason you add this to TEST_HARNESS_FILES above?

+    return [os.path.join(test['here'], f.replace('/', os.sep)) for f
             in sorted(test['head'].split(',')) if
             os.path.isfile(os.path.join(test['here'], f))]

The os.sep replace stuff should probably go back into ManifestDestiny,
but we can leave it for now.  Also, did we want to use ',' for a
separator?  I'd prefer whitespace, unless we're crazy enough to use
whitespace in (e.g.) any of our file names or other relevent
identifiers, so you can do str.split().  If you split on commas, it
should probably be 

sorted([i.strip() for i in test['head'].split(',')]) 

to protect against whitespace.  Also, why the sorted?  Should the
order in the manifest == the correct order?

+#TODO
+#      if self.testPath and not testdir.endswith(self.testPath):
+#        continue

Not sure what that's about.

+      name = os.path.abspath(name).replace('/', os.sep)

The test path is already absolute (re `name = test['path']` above)

Additionally, there seem to be a few loose-ends:

 TEST_HARNESS_FILES := \
   runxpcshelltests.py \
   remotexpcshelltests.py \
+  manifests.py \
+  xpcshell.ini \
   head.js \
   $(NULL)
 
I don't see a manifests.py here; am i missing something?

Also you do things like:

+[include:toolkit/crashreporter/test/unit/xpcshell.ini]
+os.ignore = linux
+component = mozcrashreporter

Currently this isn't used anywhere.  It should go in manifestparser.py
(or, more precisely in the TestManifest integration layer:
http://hg.mozilla.org/automation/ManifestDestiny/file/e579bfc6f1e6/manifestparser.py#l445
) but it doesn't seem like these flags are currently used.  Is this
future-proofing, or for some other intent?  We can add them in now, or
can kick the can (I've been waffling on this a bit).

f+ ; this is good.  If it were up for review I'd want to know the
answers to these questions, but I don't particularly see anything
show-stopping here.

Concurrent with checkin, we should probably note next priorities for
ManifestDestiny.  There's probably nothing big, but it would be nice
to give some sort of universalized treatment of path separators
(thanks, Mr. Gates!) and things like os.ignore, as well as several
other spec items from
https://elvis314.wordpress.com/2010/07/27/types-of-data-we-care-about-in-a-manifest/
I think if we can come up with how we would *want* to call something
like "give me all the tests for this os, platform, etc" then actually
programming it won't be too bad.  

Rough note mostly to self:

def get_tests(os=None, platform=None, ...):
Attachment #511141 - Flags: feedback?(jhammel) → feedback+
xpcshell_gen.ini is in place so I don't conflict with the checked in versions.  I removed this as it is there for me to verify my checked in stuff matches what is on the filesystem.

yes, I did cleanup a typo in an existing file.

in regards to sorting the head/tail files, that is a quirk of xpcshell.  If you put the files in the wrong order the tests don't run.

I had accidentally left manifests.py and xpcshell.ini in the TEST_HARNESS_FILES, good catch!

The big things left are resolving the os.sep thing and the todo comment for single tests or directories.  I don't know if there is an easy way around this, but I run into issues when running the tests on windows with os.path.join and the value hardcoded in the .ini file for the path.

One other comment in this f+ is regarding using a ',' vs a ' ' to separate values.  We need to resolve that in manifestparser itself first.
this patch adds xpcshell.ini files for all the xpcshell based tests in mozilla-central.  This is not a patch to read these or fix make files, this is just the manifests themselves.
Attachment #514983 - Flags: review?(jhammel)
Attachment #514983 - Flags: feedback?(ted.mielczarek)
Attached patch use manifests for xpcshell (1.0) (obsolete) — Splinter Review
patch that reads xpcshell.ini files and doesn't do directory listings or buildlist.py->all-test-dirs.list.

The one quirk with this patch is that it changes all-test-dirs.list to be xpcshell.ini.  This is easily changed back so it will work seamlessly with the releng scripts that run xpcshell on production systems.
Attachment #511141 - Attachment is obsolete: true
Attachment #514984 - Flags: review?(ted.mielczarek)
Attachment #514984 - Flags: feedback?(mak77)
Comment on attachment 514983 [details] [diff] [review]
add ini files for all xpcshell tests (1.0)

+[include:intl/locale/src/unix/tests/unit/xpcshell.ini]
+toolkit.ignore = windows, cocoa, os2

I thought we were using space separated vs comma separated. Should I change this in ManifestDestiny?  Also, somehow we should have a naming convention for these things.

What are you using `component = ipc` for, just out of curiousity?
Attachment #514983 - Flags: review?(jhammel) → review+
Comment on attachment 514983 [details] [diff] [review]
add ini files for all xpcshell tests (1.0)

>diff --git a/browser/components/dirprovider/tests/unit/xpcshell.ini b/browser/components/dirprovider/tests/unit/xpcshell.ini
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/dirprovider/tests/unit/xpcshell.ini
>@@ -0,0 +1,6 @@
>+[DEFAULT]
>+head = head_dirprovider.js
>+tail = 

Are head and tail required fields? Seems silly to have to specify an empty "tail = " for places where we don't need it.

I was originally torn on explicitly naming the head and tail files like this, since they're already determined by name, but making it more explicit might be nice, actually. Is the ordering specified in the file preserved for loading the files?

>+[test_bookmark_pref.js]
>+[test_keys.js]

Similar question here, do we preserve the order of the tests in the manifest for running them?

Without any parameters the INI format looks kind of silly, just looks like we wind up with extraneous brackets. It might take some getting used to.

>diff --git a/testing/xpcshell/xpcshell.ini b/testing/xpcshell/xpcshell.ini
>new file mode 100644
>--- /dev/null
>+++ b/testing/xpcshell/xpcshell.ini

Not to bikeshed you, but I'd probably call this "manifest.ini" or something, xpcshell is already in the directory name, we should make it slightly more clear what this is. xpcshell.ini is fine for the other files.

>+[include:layout/tools/layout-debug/tests/unit/xpcshell.ini]
>+debug = true
>+[include:intl/locale/tests_multilocale/unit/xpcshell.ini]
>+toolkit = windows

I have to say, now that I read the syntax here, I'm really not in love with it. It's not clear at all from a cursory glance that the lines below the include are associated with that line, or that they're conditions on the include. I don't have a better suggestion right now, but I'm not sure I want to land this as-is.
Attachment #514983 - Flags: feedback?(ted.mielczarek) → feedback-
Comment on attachment 514984 [details] [diff] [review]
use manifests for xpcshell (1.0)

># HG changeset patch
># Parent c51bdf942e8c7f0acd3f409d029a230433b53a0a
>Bug 616999. Xpcshell manifest support.  try: -p linux,linux64,macosx,macosx64,win32,win64 -t none
>
>diff --git a/build/automation-build.mk b/build/automation-build.mk
>--- a/build/automation-build.mk
>+++ b/build/automation-build.mk
>@@ -23,16 +23,46 @@ AUTOMATION_PPARGS += -DIS_MAC=0
> endif
> 
> ifeq ($(OS_ARCH),Linux)
> AUTOMATION_PPARGS += -DIS_LINUX=1
> else
> AUTOMATION_PPARGS += -DIS_LINUX=0
> endif
> 
>+ifeq ($(OS_ARCH),WINNT)
>+AUTOMATION_PPARGS += -DIS_WIN=1
>+else
>+AUTOMATION_PPARGS += -DIS_WIN=0
>+endif

Seems silly to propogate these. Let's just change this to -DOS_ARCH="$(OS_ARCH)", and -DWIDGET_TOOLKIT="$(MOZ_WIDGET_TOOLKIT)". If we have to change some #ifdef FOO to if FOO == "foo": in the Python, then let's do that. I don't actually see where you're using these in this patch, though, honestly.

>diff --git a/build/manifestparser.py b/build/manifestparser.py
>new file mode 100755

I'm not going to review the manifest parser code yet, since a) it's probably been looked over quite a bit already, and b) I'll probably just be picking Python nits as usual anyway, and there isn't likely to be anything so egregious that it would give you an r-. (Also it will take me a lot of time.)

>diff --git a/config/rules.mk b/config/rules.mk
>--- a/config/rules.mk
>+++ b/config/rules.mk
>@@ -165,19 +165,17 @@ define _INSTALL_TESTS
> $(TEST_INSTALLER) $(wildcard $(srcdir)/$(dir)/*) $(testxpcobjdir)/$(relativesrcdir)/$(dir)
> 
> endef # do not remove the blank line!
> 
> SOLO_FILE ?= $(error Specify a test filename in SOLO_FILE when using check-interactive or check-one)
> 
> libs::
> 	$(foreach dir,$(XPCSHELL_TESTS),$(_INSTALL_TESTS))
>-	$(PYTHON) $(MOZILLA_DIR)/config/buildlist.py \
>-	  $(testxpcobjdir)/all-test-dirs.list \
>-	  $(addprefix $(relativesrcdir)/,$(XPCSHELL_TESTS))
>+	@(cp $(topsrcdir)/testing/xpcshell/xpcshell.ini $(testxpcobjdir))

You want to use $(INSTALL) here, since that uses nsinstall under the hood, and it does a lot more than cp. Also, you don't need the extra parentheses around the command. Also, this libs:: command gets run for every single directory in the tree with XPCSHELL_TESTS defined. Do you really want to copy the top-level manifest every single time? Seems like you probably want to copy it from the testing/xpcshell Makefile.

>diff --git a/testing/xpcshell/Makefile.in b/testing/xpcshell/Makefile.in
>--- a/testing/xpcshell/Makefile.in
>+++ b/testing/xpcshell/Makefile.in
>@@ -47,27 +47,29 @@ MODULE		= testing_xpcshell
> 
> # Here's how you let the build system know there are tests in the
> # "example" folder:
> ifdef ENABLE_TESTS
> DIRS  += example
> endif
> 
> include $(topsrcdir)/config/rules.mk
>+include $(topsrcdir)/build/automation-build.mk

What's this being used for? The xpcshell harness doesn't use automation.py, it seems weird to require it to get copied around.

> stage-package:
> 	$(NSINSTALL) -D $(PKG_STAGE)/xpcshell/tests
> 	@(cd $(srcdir) && tar $(TAR_CREATE_FLAGS) - $(TEST_HARNESS_FILES)) | (cd $(PKG_STAGE)/xpcshell && tar -xf -)
>+	@(cp $(srcdir)/xpcshell.ini $(PKG_STAGE)/xpcshell/tests)

Doesn't the xpcshell.ini file already get copied to the _tests/xpcshell directory elsewhere? Why are you copying it again here?

> 	@(cd $(topsrcdir)/build && tar $(TAR_CREATE_FLAGS) - $(EXTRA_BUILD_FILES)) | (cd $(PKG_STAGE)/xpcshell && tar -xf -)
> 	@(cd $(topsrcdir)/build/mobile && tar $(TAR_CREATE_FLAGS) - $(MOBILE_BUILD_FILES)) | (cd $(PKG_STAGE)/xpcshell && tar -xf -)
> 	@(cd $(DEPTH)/_tests/xpcshell/ && tar $(TAR_CREATE_FLAGS) - *) | (cd $(PKG_STAGE)/xpcshell/tests && tar -xf -)
> 	@(cd $(DIST)/bin/components && tar $(TAR_CREATE_FLAGS) - $(TEST_HARNESS_COMPONENTS)) | (cd $(PKG_STAGE)/bin/components && tar -xf -)
>+

Kill the extra blank line you added here.

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>   def buildTestList(self):
>-    """
>-      Builds a dict of {"testdir" : ["testfile1", "testfile2", ...], "testdir2"...}.
>-      If manifest is given override testdirs to build initial list of directories and tests.
>-      If testpath is given, use that, otherwise chunk if requested.
>-      The resulting set of tests end up in self.alltests
>-    """

Please replace the docstring comment with something that matches what you're doing here instead of just removing it.

>+    mp = manifestparser.ManifestParser(strict=False)
>+    if (self.manifest is None):
>+      [mp.read(os.path.join(testdir, 'xpcshell.ini')) for testdir in self.testdirs if testdir != '']

Don't use a list comprehension for looping if you're not actually using the resulting list. If you just want to loop over self.testdirs, write a for loop.

>-  def getHeadFiles(self, testdir):
>+
>+  def getHeadFilesIni(self, test):

Seems like renaming the function is unnecessary, it's still getting the list of head files for a test.

>     """
>       Get the list of head files for a given test directory.
>       On a remote system, this is overloaded to list files in a remote directory structure.
>     """
>-    return [f for f in sorted(glob(os.path.join(testdir, "head_*.js"))) if os.path.isfile(f)]
>+    return [os.path.join(test['here'], f.replace('/', os.sep)).strip() for f in sorted(test['head'].split(',')) if os.path.isfile(os.path.join(test['here'], f))]

I'm having a really hard time figuring out what this actually does. Could you add a comment? Also, correct the docstring comment while you're here.

> 
>-  def getTailFiles(self, testdir):
>+  def getTailFilesIni(self, test):

Same comments apply here.

>@@ -451,94 +413,97 @@ class XPCShellTests(object):
> 
>     self.setAbsPath()
>     self.buildXpcsRunArgs()
>     self.buildEnvironment()
>     pStdout, pStderr = self.getPipes()
> 
>     self.buildTestList()
> 
>-    for testdir in sorted(self.alltests.keys()):
>-      if self.testPath and not testdir.endswith(self.testPath):
>+    for test in self.alltests:
>+      name = test['path']
>+      if self.singleFile and not name.endswith(self.singleFile):
>         continue
> 
>+      if self.testPath and name.find(self.testPath) == -1:
>+        continue
>+
>+      testdir = os.sep.join(name.split(os.sep)[0:-1])

Ick. Isn't this just os.dirname()? Also, why are we using os.sep? We should just say that paths in manifests use forward slashes.

>diff --git a/toolkit/mozapps/extensions/test/Makefile.in b/toolkit/mozapps/extensions/test/Makefile.in
>--- a/toolkit/mozapps/extensions/test/Makefile.in
>+++ b/toolkit/mozapps/extensions/test/Makefile.in
>@@ -70,9 +70,10 @@ libs::
> 	if [ -d $(ADDONSRC) ]; then \
> 		$(EXIT_ON_ERROR) \
> 		for dir in $(ADDONSRC)/*; do \
> 			base=`basename $$dir` ; \
> 			(cd $$dir && zip -r $(TESTXPI)/$$base.xpi *) \
> 		done \
> 	fi
> 	cd $(TESTROOT)/xpcshell/ && tar -cPf - . | (cd $(TESTROOT)/xpcshell-unpack && tar -xPvf - )
>-
>+	@(sed s/head_addons.js/head_addons.js,head_unpack.js/ $(TESTROOT)/xpcshell-unpack/xpcshell.ini > xpcshell.in_)
>+	mv xpcshell.in_ $(TESTROOT)/xpcshell-unpack/xpcshell.ini

This is so awful, but I don't have a better suggestion right now. I would say that you should just pipe the output directly to $(TESTROOT)/xpcshell-unpack/xpcshell.ini instead of having that extra mv. Also, you don't need the parentheses around your sed command. (If you're cargo-culting them from other lines, the reason we have them in some of those lines with "tar" is that we want to cd && tar in one subshell, and then pipe the output of that into another subshell where we cd && tar, so we use them to group the subshells appropriately.)

r- for some things that need to be addressed.
Attachment #514984 - Flags: review?(ted.mielczarek) → review-
(In reply to comment #14)
> >+[test_bookmark_pref.js]
> >+[test_keys.js]
> 
> Similar question here, do we preserve the order of the tests in the manifest
> for running them?

Yes.
 
> Without any parameters the INI format looks kind of silly, just looks like we
> wind up with extraneous brackets. It might take some getting used to.

Everyone agrees and yet no one has proposed a better format abict. We can write our own format, something like

test_keys.js
: foo
: bar = fleem

which will avoid the extraneous brackets problem.  INI format was chosen because 
- people hate JSON
- people hate YAML
- its at least a familiar format, even if we're using it in an unfamiliar way (e.g. ordered)
- no one else could decide what to do

If there is a better format that achieves the same objectives, we should consider this, but the train is coming close to leaving the station.

> >diff --git a/testing/xpcshell/xpcshell.ini b/testing/xpcshell/xpcshell.ini
> >new file mode 100644
> >--- /dev/null
> >+++ b/testing/xpcshell/xpcshell.ini
> 
> Not to bikeshed you, but I'd probably call this "manifest.ini" or something,
> xpcshell is already in the directory name, we should make it slightly more
> clear what this is. xpcshell.ini is fine for the other files.

FWIW, I call the manifests in Mozmill manifest.ini

> >+[include:layout/tools/layout-debug/tests/unit/xpcshell.ini]
> >+debug = true
> >+[include:intl/locale/tests_multilocale/unit/xpcshell.ini]
> >+toolkit = windows
> 
> I have to say, now that I read the syntax here, I'm really not in love with it.
> It's not clear at all from a cursory glance that the lines below the include
> are associated with that line, or that they're conditions on the include. I
> don't have a better suggestion right now, but I'm not sure I want to land this
> as-is.

Its an .ini file.  Like all .ini files, the key=values are associated with the above section
Comment on attachment 514984 [details] [diff] [review]
use manifests for xpcshell (1.0)

I'm not that good with python and build so I'm not sure what my feedback should be on, I'll give some first impressions.

When I originally read the title of the bug, I thought it was using the current manifests format, then a test definition would just take 1 line and be separated by spaces. And the os definitions could be defined by boolean ops, something like
test_1.js os=!win
test_2.js os=lin
test_3.js os=lin64,mac64
test_4.js toolkit=cocoa
test_5.js
The syntax for manifest files is common in the codebase, just those files probably can't be named .manifest, maybe just tests.list

The ini is not bad, but seems a bit inflated for the few variables involved. I don't see the keys => test association problem, maybe because I'm used to the ini format.

Personally I don't see much the need for specifying head and tail, do we have actual examples of the usefulness?

If you want that I take a general check at the code I could do that, but I see Ted has covered most of it.
Attachment #514984 - Flags: feedback?(mak77)
updated patch to support newer syntax and easier to read manifests.  Also updated for all tests we have in the tree.
Attachment #514983 - Attachment is obsolete: true
Attachment #531421 - Flags: review?(ted.mielczarek)
Attached patch use manifests for xpcshell (2.0) (obsolete) — Splinter Review
updated manifestdestiny and general integration into mozilla-central
Attachment #514984 - Attachment is obsolete: true
Attachment #531422 - Flags: review?(ted.mielczarek)
this is the final patch in order to run.  The bulk of this is a sanity checker that will throw an error during make to indicate a xpcshell file is added/removed from a directory, but not a manifest (.ini) file.

In addition, this hacks a 'cp xpcshell.ini all-test-dirs.list', so we can run on tinderbox.
Attachment #531424 - Flags: review?(ted.mielczarek)
Comment on attachment 531421 [details] [diff] [review]
add ini files for all xpcshell tests (2.0)

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

I have no major qualms with this. I'm sure we'll get plenty of feedback after this lands, though!
Attachment #531421 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 531422 [details] [diff] [review]
use manifests for xpcshell (2.0)

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

I don't think you actually fixed most of the review comments from my last review. Can you go back and do that so I don't have to make the same comments again?

::: config/rules.mk
@@ +141,5 @@
>  SOLO_FILE ?= $(error Specify a test filename in SOLO_FILE when using check-interactive or check-one)
>  
>  libs::
>  	$(foreach dir,$(XPCSHELL_TESTS),$(_INSTALL_TESTS))
> +	@(cp $(topsrcdir)/testing/xpcshell/xpcshell.ini $(testxpcobjdir))

I feel like I commented on this previously, but you don't want to do this in rules.mk, because you're going to copy this file once for every directory with tests.
(Also, use $(INSTALL), not cp, and the @() are unnecessary.)

::: testing/xpcshell/Makefile.in
@@ +85,5 @@
>  
>  stage-package:
>  	$(NSINSTALL) -D $(PKG_STAGE)/xpcshell/tests
>  	@(cd $(srcdir) && tar $(TAR_CREATE_FLAGS) - $(TEST_HARNESS_FILES)) | (cd $(PKG_STAGE)/xpcshell && tar -xf -)
> +	@(cp $(srcdir)/xpcshell.ini $(PKG_STAGE)/xpcshell/tests)

You don't need the extra () here, you only need those if you're doing a pipeline of commands.
Attachment #531422 - Flags: review?(ted.mielczarek) → review-
Comment on attachment 531424 [details] [diff] [review]
manifest integrity checker and hooks to work with tinderbox scripts (1.0)

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

Just a few nitpicky things to address, looks good otherwise.

::: build/xpccheck.py
@@ +56,5 @@
> +  for f in files:
> +    if (not os.path.isfile(f)):
> +      continue
> +
> +    name = f.split('/')[-1]

os.path.basename(f)

@@ +58,5 @@
> +      continue
> +
> +    name = f.split('/')[-1]
> +    if name.endswith('.in'):
> +      name = '.in'.join(name.split('.in')[:-1])

You just want:
name = name[:-3]

@@ +70,5 @@
> +        found = True
> +        break
> +   
> +    if (not found):
> +      print >>sys.stderr, "unable to find '%s' in '%s'" % (name, os.path.join(directory, 'xpcshell.ini'))

Might want to make this clearer, like:
Error: test %s is missing from test manifest %s!

Maybe even throw a TEST-UNEXPECTED-FAIL at the beginning so it shows up in the error logs clearly?

@@ +90,5 @@
> +        found = True
> +        break
> +
> +    if (not found):
> +      print >>sys.stderr, "unable to find '%s' in directory '%s'" % (name, directory)

Same thing here.

::: config/rules.mk
@@ +141,5 @@
>  SOLO_FILE ?= $(error Specify a test filename in SOLO_FILE when using check-interactive or check-one)
>  
>  libs::
>  	$(foreach dir,$(XPCSHELL_TESTS),$(_INSTALL_TESTS))
> +	  $(PYTHON) $(MOZILLA_DIR)/build/xpccheck.py \

You have an extra two spaces before $(PYTHON) here.

::: testing/testsuite-targets.mk
@@ +164,5 @@
>  xpcshell-tests:
>  	$(PYTHON) -u $(topsrcdir)/config/pythonpath.py \
>  	  -I$(topsrcdir)/build \
>  	  $(topsrcdir)/testing/xpcshell/runxpcshelltests.py \
> +	  --manifest=$(DEPTH)/_tests/xpcshell/all-test-dirs.list \

Any particular reason you changed this back? You could just copy it to the old name in the packaging step for backwards-compat, but leave it in the objdir.
Attachment #531424 - Flags: review?(ted.mielczarek) → review+
updated manifest files for latest checkins
Attachment #531421 - Attachment is obsolete: true
Attachment #532822 - Flags: review+
I know this was r+ before, but I made some updates that are not super trivial, also found a big hole where I was not sanity checking the actual master list of manifests, only checking the directories and their manifests.
Attachment #531424 - Attachment is obsolete: true
Attachment #532823 - Flags: review?(ted.mielczarek)
update with feedback for real this time!
Attachment #531422 - Attachment is obsolete: true
Attachment #532824 - Flags: review?(ted.mielczarek)
Comment on attachment 532823 [details] [diff] [review]
manifest integrity checker and hooks to work with tinderbox scripts (2.0)

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

::: build/xpccheck.py
@@ +65,5 @@
> +      continue
> +
> +    found = False
> +    for test in initests:
> +      if test['path'].endswith(name):

Seems like you could probably compare absolute paths here or something to be more certain.

@@ +69,5 @@
> +      if test['path'].endswith(name):
> +        found = True
> +        break
> +   
> +    if (not found):

Don't need the parens here.

@@ +70,5 @@
> +        found = True
> +        break
> +   
> +    if (not found):
> +      print >>sys.stderr, "TEST-UNEXPECTED-ERROR: test %s is missing from test manifest %s!" % (name, os.path.join(directory, 'xpcshell.ini'))

TEST-UNEXPECTED-FAIL | xpccheck | test %s ...

@@ +90,5 @@
> +        found = True
> +        break
> +
> +    if (not found):
> +      print >>sys.stderr, "TEST-UNEXPECTED-ERROR: found %s in xpcshell.ini anot not in directory '%s'" % (name, directory)

Drop the parens and change the output format here like above.

@@ +95,5 @@
> +      sys.exit(1)
> +
> +def verifyMasterIni(mastername, directory):
> +  fhandle = open(mastername, 'r')
> +  masterini = fhandle.read()

Does the manifest parser not let you see what files were included?

@@ +100,5 @@
> +  fhandle.close()
> +
> +  found = False
> +  for line in masterini.split('\n'):
> +    if line.startswith('[include:' + directory.strip('/')):

Won't this fail if you have two levels deep of [include]? (Top-level manifest includes a manifest that includes another one.)

@@ +105,5 @@
> +      found = True
> +      break
> +
> +  if (not found):
> +    print >>sys.stderr, "TEST-UNEXPECTED-ERROR: directory %s is missing from master xpcshell.ini file %s" % (directory, mastername)

Parens + output format. (I guess you either copy-pasted this or you just mentally always want to put parens in. :)

::: config/rules.mk
@@ +142,5 @@
>  
>  libs::
>  	$(foreach dir,$(XPCSHELL_TESTS),$(_INSTALL_TESTS))
> +	$(PYTHON) $(MOZILLA_DIR)/build/xpccheck.py \
> +	$(topsrcdir) \

I know I asked you to un-indent $(PYTHON), but could you two-space indent the rest of the lines in this command? Makes it easier to see that they're a continuation.
Attachment #532823 - Flags: review?(ted.mielczarek) → review+
Attachment #532824 - Attachment is patch: true
Comment on attachment 532824 [details] [diff] [review]
use manifests for xpcshell (2.1)

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

::: services/crypto/component/tests/Makefile.in
@@ +39,5 @@
>  DEPTH     = ../../../..
>  topsrcdir = @top_srcdir@
>  srcdir    = @srcdir@
>  VPATH     = @srcdir@
> +relativesrcdir = services/crypto/component/tests

This makes me sad. We should really just get bug 395019 fixed.

::: testing/xpcshell/runxpcshelltests.py
@@ +73,3 @@
>      """
> +    mp = manifestparser.ManifestParser(strict=False)
> +    if (self.manifest is None):

Your parens, they are killing me.

@@ +73,5 @@
>      """
> +    mp = manifestparser.ManifestParser(strict=False)
> +    if (self.manifest is None):
> +      for testdir in self.testdirs:
> +        if testdir != '':

a) Can this ever actually be empty?
b) You can just write this as "if testdir:"

::: toolkit/mozapps/extensions/test/Makefile.in
@@ +75,5 @@
>  		done \
>  	fi
>  	cd $(TESTROOT)/xpcshell/ && tar -cPf - . | (cd $(TESTROOT)/xpcshell-unpack && tar -xPvf - )
> +	sed s/head_addons.js/head_addons.js\ head_unpack.js/ $(TESTROOT)/xpcshell-unpack/xpcshell.ini > xpcshell.in_
> +	mv xpcshell.in_ $(TESTROOT)/xpcshell-unpack/xpcshell.ini

You can just pipe the sed directly to $(TESTROOT)/xpcshell-unpack/xpcshell.ini instead.
Attachment #532824 - Flags: review?(ted.mielczarek) → review+
This broke the maemo5-gtk build:

IOError: Missing files: /home/cltbld/build/mozilla-central-mob-maemo5-gtk/mozilla-central/toolkit/crashreporter/client/maemo-unit/xpcshell.ini
Attachment #534033 - Flags: review?(ctalbert) → review+
forgot to add this to the master list of manifests.  Of course the build on try server caught it.  Tested locally to ensure we don't have any problems with a linux desktop build.
Attachment #534033 - Attachment is obsolete: true
Attachment #534064 - Flags: review?(jhammel)
Comment on attachment 534064 [details] [diff] [review]
add in xpcshell manifest file for maemo crashreporter (2.0)

looks good
Attachment #534064 - Flags: review?(jhammel) → review+
Depends on: 658666
This needs documents on what features are available in the manifests, afaik there haven't been any referenced so far, please point me in the right direction if I'm wrong.
Keywords: dev-doc-needed
(In reply to comment #33)
> This needs documents on what features are available in the manifests, afaik
> there haven't been any referenced so far, please point me in the right
> direction if I'm wrong.

README is here: http://hg.mozilla.org/automation/ManifestDestiny/file/tip/README.txt 

Update in progress.
Blocks: 658878
Comment on attachment 534064 [details] [diff] [review]
add in xpcshell manifest file for maemo crashreporter (2.0)

pushed to fix maemo build bustage
http://hg.mozilla.org/mozilla-central/rev/e736fc812884
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Landed a follow-up fix on services-central to account for the changes there since the last merge with m-c:

https://hg.mozilla.org/services/services-central/rev/275b98888211
Depends on: 659881
You need to log in before you can comment on or make changes to this bug.