Closed Bug 844655 Opened 7 years ago Closed 7 years ago

Define xpcshell manifests in moz.build files

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla23

People

(Reporter: gps, Assigned: joey)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

We should replace XPCSHELL_TESTS from Makefile.in with a XPCSHELL_MANIFESTS (or similar) in moz.build files.

Once this is done, we should theoretically also be able to get rid of the master testing/xpcshell/xpcshell*.ini manifest files. We may still want master manifests for the test runner - we can generate them automatically now! I believe that since we ignore [include]d files from the master manifest, there should be little to no risk doing this. Looser coupling - yay!
This might have implications for global manifest work. We're not proposing anything radical here - just moving variable from Makefile.in to moz.build.
one downside of getting rid of this is we have an android manifest which is a lot easier to maintain instead of editing hundreds of files to disable tests.
Makefile.in and moz.build edits from script conversion.
A few stray comments were added from nested if conditionals - extra will be better than omission at this point.
Try job passed.  Running tests locally reports 20 failures.  Have to review test logs and see if they are being run/filtered.
Same patch as last time, just attached unit test data files.
Attachment #736493 - Attachment is obsolete: true
Attachment #737007 - Flags: review?(gps)
Attachment #737007 - Flags: review?(gps) → review?(mshal)
Comment on attachment 736426 [details] [diff] [review]
var migration from Makefile.in to moz.build

First pass at variable migration, renamed to 'DISABLED_' to simplify rebasing.  Can be removed after the initial landing.

A few stray comments from nested includes were propogated and can be cleaned up later.
Attachment #736426 - Flags: review?(mshal)
Comment on attachment 736426 [details] [diff] [review]
var migration from Makefile.in to moz.build

Overall it looks very close. Just some minor fixes for comments / conditional statements. I think the only (possibly) major thing is the todo in dom/indexedDB/ipc/, but I'm not sure what to do there.

>diff --git a/dom/indexedDB/ipc/Makefile.in b/dom/indexedDB/ipc/Makefile.in
>--- a/dom/indexedDB/ipc/Makefile.in
>+++ b/dom/indexedDB/ipc/Makefile.in
>@@ -38,14 +38,17 @@ MOCHITEST_FILES = test_ipc.html
> 
> # We're copying tests from another directory so this check is wrong for us.
> #NO_XPCSHELL_MANIFEST_CHECK = 1
> 
> include $(topsrcdir)/config/config.mk
> include $(topsrcdir)/ipc/chromium/chromium-config.mk
> include $(topsrcdir)/config/rules.mk
> 
>+# 844655 todo: logic not handled by conversion script so massage.
>+xpcshell_tests = unit

You still have a "todo" here. Can we just access the variable defined by backend.mk here?

>+
> # Copy all the normal xpcshell tests from the regular unit directory.
> copy-xpcshell-tests:
> 	$(call install_cmd,$(wildcard $(topsrcdir)/dom/indexedDB/test/unit/test_*.js) \
>-		$(testxpcobjdir)/$(relativesrcdir)/$(XPCSHELL_TESTS))
>+		$(testxpcobjdir)/$(relativesrcdir)/$(xpcshell_tests))

Any idea what the point of copying these tests is? Do they need to be run twice for some reason?

>diff --git a/image/test/moz.build b/image/test/moz.build
>--- a/image/test/moz.build
>+++ b/image/test/moz.build
>@@ -3,8 +3,10 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> TEST_DIRS += ['mochitest', 'browser']
> 
> MODULE = 'test_libpr0n'
> 
>+# Module name for xpcshell tests.

Can probably get rid of this comment - the help text comes from sandbox_symbols.py, and does not need to be in every moz.build file. This comment doesn't add anything specific to image/test/.

>diff --git a/modules/libpref/test/moz.build b/modules/libpref/test/moz.build
>--- a/modules/libpref/test/moz.build
>+++ b/modules/libpref/test/moz.build
>@@ -1,8 +1,14 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> MODULE = 'test_libpref'
> 
>+# FIXME/bug 575918: out-of-process xpcshell is broken on OS X
>+XPCSHELL_MANIFEST += ['unit']

The above comment shouldn't be here, only the comment for the if-statement
below.

>+
>+# FIXME/bug 575918: out-of-process xpcshell is broken on OS X
>+if CONFIG['OS_ARCH'] != 'Darwin':
>+    XPCSHELL_MANIFEST += ['unit_ipc']

>diff --git a/netwerk/cookie/moz.build b/netwerk/cookie/moz.build
>--- a/netwerk/cookie/moz.build
>+++ b/netwerk/cookie/moz.build
>@@ -12,8 +12,15 @@ XPIDL_SOURCES += [
>     'nsICookiePermission.idl',
>     'nsICookieService.idl',
> ]
> 
> XPIDL_MODULE = 'necko_cookie'
> 
> MODULE = 'necko'
> 
>+# FIXME/bug 575918: out-of-process xpcshell is broken on OS X
>+if CONFIG['NECKO_COOKIES']:
>+    XPCSHELL_MANIFEST += ['test/unit']
>+
>+# FIXME/bug 575918: out-of-process xpcshell is broken on OS X
>+if CONFIG['NECKO_COOKIES'] and CONFIG['OS_ARCH'] != 'Darwin':
>+    XPCSHELL_MANIFEST += ['test/unit_ipc']

Same here - don't have the FIXME comment on the NECKO_COOKIES line.

Also, why not do:

if CONFIG['NECKO_COOKIES']:
    XPCSHELL_MANIFEST += ['test/unit']

    if and CONFIG['OS_ARCH'] != 'Darwin':
        XPCSHELL_MANIFEST += ['test/unit_ipc']

>diff --git a/netwerk/test/moz.build b/netwerk/test/moz.build
>--- a/netwerk/test/moz.build
>+++ b/netwerk/test/moz.build
>@@ -3,8 +3,14 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> TEST_DIRS += ['httpserver', 'browser']
> 
> MODULE = 'test_necko'
> 
>+# FIXME/bug 575918: out-of-process xpcshell is broken on OS X
>+XPCSHELL_MANIFEST += ['unit']
>+
>+# FIXME/bug 575918: out-of-process xpcshell is broken on OS X
>+if CONFIG['OS_ARCH'] != 'Darwin':
>+    XPCSHELL_MANIFEST += ['unit_ipc']

Another duplicated comment.

>diff --git a/toolkit/components/commandlines/test/moz.build b/toolkit/components/commandlines/test/moz.build
>--- a/toolkit/components/commandlines/test/moz.build
>+++ b/toolkit/components/commandlines/test/moz.build
>@@ -1,8 +1,15 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> MODULE = 'test_harness_commandlines'
> 
>+XPCSHELL_MANIFEST += ['unit']
>+
>+if CONFIG['OS_ARCH'] == 'WINNT':
>+    XPCSHELL_MANIFEST += ['unit_win']
>+
>+if CONFIG['OS_ARCH'] != 'WINNT' and CONFIG['OS_ARCH'] not in ['Darwin', 'OS2']:
>+    XPCSHELL_MANIFEST += ['unit_unix']

Just use an else instead of if CONFIG['OS_ARCH'] != 'WINNT'.

Also, other multi-CONFIG checks are using () instead of []. Not sure if that really matters, but suggest using this for consistency (untested):

if CONFIG['OS_ARCH'] == 'WINNT':
    XPCSHELL_MANIFEST += ['unit_win']
elif CONFIG['OS_ARCH'] not in ('Darwin', 'OS2'):
    XPCSHELL_MANIFEST += ['unit_unix']

>diff --git a/toolkit/components/contentprefs/tests/moz.build b/toolkit/components/contentprefs/tests/moz.build
>--- a/toolkit/components/contentprefs/tests/moz.build
>+++ b/toolkit/components/contentprefs/tests/moz.build
>@@ -1,8 +1,14 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> MODULE = 'test_toolkit_contentprefs'
> 
>+# FIXME/bug 575918: out-of-process xpcshell is broken on OS X
>+XPCSHELL_MANIFEST += ['unit', 'unit_cps2']
>+
>+# FIXME/bug 575918: out-of-process xpcshell is broken on OS X
>+if CONFIG['OS_ARCH'] != 'Darwin':
>+    XPCSHELL_MANIFEST += ['unit_ipc']

Another duplicate comment.

>diff --git a/toolkit/components/passwordmgr/test/moz.build b/toolkit/components/passwordmgr/test/moz.build
>--- a/toolkit/components/passwordmgr/test/moz.build
>+++ b/toolkit/components/passwordmgr/test/moz.build
>@@ -3,8 +3,10 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> TEST_DIRS += ['browser', 'auth2']
> 
> MODULE = 'test_passwordmgr'
> 
>+# Module name for xpcshell tests.
>+XPCSHELL_MANIFEST += ['unit']

Suggest removing this comment as well (doesn't provide any extra information specific to this directory).

>diff --git a/uriloader/exthandler/tests/moz.build b/uriloader/exthandler/tests/moz.build
>--- a/uriloader/exthandler/tests/moz.build
>+++ b/uriloader/exthandler/tests/moz.build
>@@ -3,8 +3,14 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> DIRS += ['mochitest']
> 
> MODULE = 'test_uriloader_exthandler'
> 
>+#FIXME/bug 575918: out-of-process xpcshell is broken on OS X
>+XPCSHELL_MANIFEST += ['unit']
>+
>+#FIXME/bug 575918: out-of-process xpcshell is broken on OS X
>+if CONFIG['OS_ARCH'] != 'Darwin':
>+    XPCSHELL_MANIFEST += ['unit_ipc']

Another duplicate comment.
Attachment #736426 - Flags: review?(mshal) → feedback+
Comment on attachment 737007 [details] [diff] [review]
python/mozbuild logic for XPCSHELL_MANIFEST

>diff --git a/python/mozbuild/mozbuild/frontend/data.py b/python/mozbuild/mozbuild/frontend/data.py
>--- a/python/mozbuild/mozbuild/frontend/data.py
>+++ b/python/mozbuild/mozbuild/frontend/data.py
>@@ -143,8 +143,18 @@ class Exports(SandboxDerived):
>     this object fills that role. It just has a reference to the underlying
>     HierarchicalStringList, which is created when parsing EXPORTS.
>     """
>     __slots__ = ('exports')
> 
>     def __init__(self, sandbox, exports):
>         SandboxDerived.__init__(self, sandbox)
>         self.exports = exports
>+
>+class XpcshellManifest(SandboxDerived):
>+    """
>+    """
>+
>+    __slots__ = ('xpcshell_manifest')
>+
>+    def __init__(self, sandbox, manifest):
>+        SandboxDerived.__init__(self, sandbox)
>+        self.xpcshell_manifest = manifest

Do we need this XpcshellManifest class for anything in particular? I think this would be a good candidate for a passthru variable, similar to XPIDL_FLAGS. Eg:

@@ -84,6 +83,8 @@ class TreeMetadataEmitter(object):
             passthru.variables['XPIDL_MODULE'] = sandbox['XPIDL_MODULE']
         if sandbox['XPIDL_FLAGS']:
             passthru.variables['XPIDL_FLAGS'] = sandbox['XPIDL_FLAGS']
+        if sandbox['XPCSHELL_MANIFEST']:
+            passthru.variables['XPCSHELL_TESTS'] = sandbox['XPCSHELL_MANIFEST']

Then we wouldn't need the extra class, or _process_xpcshell_manifest().

If we do need this for something (future plans?) then I think all we need is a non-empty comment for the XpcshellManifest class.
Attachment #737007 - Flags: review?(mshal) → review-
(In reply to Michael Shal [:mshal] from comment #7)
> Comment on attachment 736426 [details] [diff] [review]
> >+
> > # Copy all the normal xpcshell tests from the regular unit directory.
> > copy-xpcshell-tests:
> > 	$(call install_cmd,$(wildcard $(topsrcdir)/dom/indexedDB/test/unit/test_*.js) \
> >-		$(testxpcobjdir)/$(relativesrcdir)/$(XPCSHELL_TESTS))
> >+		$(testxpcobjdir)/$(relativesrcdir)/$(xpcshell_tests))
> 
> Any idea what the point of copying these tests is? Do they need to be run
> twice for some reason?

I believe these tests are also run in mochitests with a thin .html wrapper around them.
(In reply to Michael Shal [:mshal] from comment #7)
> Comment on attachment 736426 [details] [diff] [review]
> var migration from Makefile.in to moz.build
> 
> Overall it looks very close. Just some minor fixes for comments /
> conditional statements. I think the only (possibly) major thing is the todo
> in dom/indexedDB/ipc/, but I'm not sure what to do there.

> >+# 844655 todo: logic not handled by conversion script so massage.
> >+xpcshell_tests = unit
> 
> You still have a "todo" here. Can we just access the variable defined by
> backend.mk here?
> 
> Any idea what the point of copying these tests is? Do they need to be run
> twice for some reason?

The 'todo' here was added as a placeholder.  Land the bulk variable translations & logic.  Then revisit the creepie-crawlies as cleanup in a followup patch.



> >+# FIXME/bug 575918: out-of-process xpcshell is broken on OS X
> >+XPCSHELL_MANIFEST += ['unit']
> 
> The above comment shouldn't be here, only the comment for the if-statement
> below.

These are some of the 'stray comments' that came through when the nested if blocks were parsed.  To be cleaned up in a followup job once re-basing is out of the picture.
(In reply to Michael Shal [:mshal] from comment #7)
> Comment on attachment 736426 [details] [diff] [review]
> var migration from Makefile.in to moz.build
> 
> Also, why not do:
> 
> if CONFIG['NECKO_COOKIES']:
>     XPCSHELL_MANIFEST += ['test/unit']
> 
>     if and CONFIG['OS_ARCH'] != 'Darwin':
>         XPCSHELL_MANIFEST += ['test/unit_ipc']

The conditionals are being programatically extracted/converted so some may be written out more explicitly than they need to be.  Post conversion landing they can be simplified.
we would rather have all the logic in the manifests vs mixed in makefile and moz.build files.  Obviously if a parent directory is not included in a build based on a build configuration, then we will not get the tests in a subfolder from that feature.
(In reply to Michael Shal [:mshal] from comment #8)
> Comment on attachment 737007 [details] [diff] [review]
> python/mozbuild logic for XPCSHELL_MANIFEST
> 
> >diff --git a/python/mozbuild/mozbuild/frontend/data.py b/python/mozbuild/mozbuild/frontend/data.py
> >--- a/python/mozbuild/mozbuild/frontend/data.py
> >+++ b/python/mozbuild/mozbuild/frontend/data.py
> >@@ -143,8 +143,18 @@ class Exports(SandboxDerived):
> >     this object fills that role. It just has a reference to the underlying
> >     HierarchicalStringList, which is created when parsing EXPORTS.
> >     """
> >     __slots__ = ('exports')
> > 
> >     def __init__(self, sandbox, exports):
> >         SandboxDerived.__init__(self, sandbox)
> >         self.exports = exports
> >+
> >+class XpcshellManifest(SandboxDerived):
> >+    """
> >+    """
> >+
> >+    __slots__ = ('xpcshell_manifest')
> >+
> >+    def __init__(self, sandbox, manifest):
> >+        SandboxDerived.__init__(self, sandbox)
> >+        self.xpcshell_manifest = manifest
> 
> Do we need this XpcshellManifest class for anything in particular? I think
> this would be a good candidate for a passthru variable, similar to
> XPIDL_FLAGS. Eg:
> 
> @@ -84,6 +83,8 @@ class TreeMetadataEmitter(object):
>              passthru.variables['XPIDL_MODULE'] = sandbox['XPIDL_MODULE']
>          if sandbox['XPIDL_FLAGS']:
>              passthru.variables['XPIDL_FLAGS'] = sandbox['XPIDL_FLAGS']
> +        if sandbox['XPCSHELL_MANIFEST']:
> +            passthru.variables['XPCSHELL_TESTS'] =
> sandbox['XPCSHELL_MANIFEST']
> 
> Then we wouldn't need the extra class, or _process_xpcshell_manifest().
> 
> If we do need this for something (future plans?) then I think all we need is
> a non-empty comment for the XpcshellManifest class.

From the comment attached to variablePassthrough() we probably still need it:

class VariablePassthru(SandboxDerived):
    """A dict of variables to pass through to backend.mk unaltered.

    The purpose of this object is to facilitate rapid transitioning of
    variables from Makefile.in to moz.build. In the ideal world, this class
    does not exist and every variable has a richer class representing it.
    As long as we rely on this class, we lose the ability to have flexibility
    in our build backends since we will continue to be tied to our rules.mk.
    """
Eventually I see the build system peeking inside test manifest files at config.status time and deriving build actions from what it finds. e.g. when it encounters XPCSHELL_TEST_MANIFESTS (or whatever the variable is named), it calls out to the xpcshell.ini parser (implemented in Python) and figures out what test files are present, what resource files it defines, etc. It then passes this metadata down to the build backend, which creates the necessary "install these files" rules.

At that time, we're likely looking at a dedicated class to convey the rich metadata within the test manifest.

But, we're a long way away from there.

For the initial landing, let's just capture the filenames of xpcshell test manifests and convert that set to a passthru variable (e.g. by taking the directory name). Later, we can add deep parsing of manifests and enhanced build system rules while preserving the public moz.build API.

Make sense?
Attachment #736426 - Flags: review?(gps)
Attachment #736426 - Flags: review?(gps)
(In reply to Gregory Szorc [:gps] from comment #14)
> Eventually I see the build system peeking inside test manifest files at
> config.status time and deriving build actions from what it finds. e.g. when
> it encounters XPCSHELL_TEST_MANIFESTS (or whatever the variable is named),
> it calls out to the xpcshell.ini parser (implemented in Python) and figures
> out what test files are present, what resource files it defines, etc. It
> then passes this metadata down to the build backend, which creates the
> necessary "install these files" rules.
> 
> At that time, we're likely looking at a dedicated class to convey the rich
> metadata within the test manifest.
> 
> But, we're a long way away from there.
> 
> For the initial landing, let's just capture the filenames of xpcshell test
> manifests and convert that set to a passthru variable (e.g. by taking the
> directory name). Later, we can add deep parsing of manifests and enhanced
> build system rules while preserving the public moz.build API.
> 
> Make sense?

If we know a class will eventually be needed to support enhanced parsing functionality and the attached patch already implements proper behavior {according to the passthrough function comments}.  Why not just define classes for these variables at the start.  It would create a framework that people can more easily extend later on.
Do whatever is the least work for you *now*. If I were starting from scratch I would use passthru variables since I'm lazy. But if your patch already uses a class, it's probably easier to continue using that class.
Assignee: nobody → joey
Attachment #736426 - Flags: review?(gps)
(In reply to Michael Shal [:mshal] from comment #7)
> Comment on attachment 736426 [details] [diff] [review]
> var migration from Makefile.in to moz.build

> >diff --git a/image/test/moz.build b/image/test/moz.build
> >--- a/image/test/moz.build
> >+++ b/image/test/moz.build
> >@@ -3,8 +3,10 @@
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # License, v. 2.0. If a copy of the MPL was not distributed with this
> > # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> > TEST_DIRS += ['mochitest', 'browser']
> > 
> > MODULE = 'test_libpr0n'
> > 
> >+# Module name for xpcshell tests.
> 
> Can probably get rid of this comment - the help text comes from
> sandbox_symbols.py, and does not need to be in every moz.build file. This
> comment doesn't add anything specific to image/test/.

[fyi] the comment line was taken verbatim from Makefile.in by automated conversion.  This one can be filtered out easily enough but assumptions are not being made about if-block comment content they are simply being transferred.
Filter '# Module name for xpcshell tests' comment.
Conversion logic updated to report a comment once for nested if blocks to reduce the frequency of duplicate and stray comments.
multi-CONFIG check(s) updated, should have been written using parens rather than braces.

todo items to be handled in a followup patch:
  dom/indexdDB/ipc/Makefile.in
  conditional optimizations
Attachment #736426 - Attachment is obsolete: true
Attachment #736426 - Flags: review?(gps)
Attachment #738535 - Flags: review?(mshal)
Let's try this again but include makefile edits.  A local edit in the conversion script prevented generation.
Attachment #738535 - Attachment is obsolete: true
Attachment #738535 - Flags: review?(mshal)
Attachment #738625 - Flags: review?(mshal)
Fix one stray variable edit: XPCSHELL_TESTS_COMMON
Attachment #738625 - Attachment is obsolete: true
Attachment #738625 - Flags: review?(mshal)
Attachment #738633 - Flags: review?(mshal)
Comment on attachment 738633 [details] [diff] [review]
moz.build XPCSHELL_TEST var migration

The comment for why XPCSHELL_MANIFEST is commented out in dom/indexedDB/ipc/ needs to be carried over.

This looks good to me, assuming the followup patch includes removing the DISABLED_XPCSHELL_TESTS lines from Makefile.in (in addition to the conditionals and dom/indexedDB/ipc/ changes you mention).
Attachment #738633 - Flags: review?(mshal) → review+
Comment on attachment 737007 [details] [diff] [review]
python/mozbuild logic for XPCSHELL_MANIFEST

I should still probably look at this...
Attachment #737007 - Flags: review?(gps)
fixed mshal's nit: added comment for Xpcshell class
Attachment #737007 - Attachment is obsolete: true
Attachment #737007 - Flags: review?(gps)
Attachment #739022 - Flags: review?(gps)
(In reply to Gregory Szorc [:gps] from comment #22)
> Comment on attachment 737007 [details] [diff] [review]
> python/mozbuild logic for XPCSHELL_MANIFEST
> 
> I should still probably look at this...

fyi - This patch has not changed much from the upload on the 12th
Comment on attachment 739022 [details] [diff] [review]
bug 844655: Port XPCSHELL_TESTS to moz.build as XPCSHELL_MANIFEST

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +325,5 @@
>              self._process_exports(children[subdir], backend_file,
>                                    namespace=namespace + subdir)
> +
> +    def _process_xpcshell_manifest(self, manifest, backend_file, namespace=""):
> +        backend_file.write('XPCSHELL_TESTS += %s\n' % (' '.join(manifest)))

I'm not sure if I'd bother with a one line function at this time. Not a blocker though.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +93,5 @@
>              yield Exports(sandbox, exports)
>  
> +        manifest = sandbox.get('XPCSHELL_MANIFEST')
> +        if manifest:
> +            yield XpcshellManifest(sandbox, manifest)

I would emit a separate object for each entry in the list. They are independent, so they should not be treated as a unit, right?

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +195,5 @@
> +    'XPCSHELL_MANIFEST': (list, [],
> +        """XPCSHELL testing manifest.
> +
> +        A list of directories containing testing manifest files for xpcshell.
> +        Formerly XPCSHELL_TESTS=

Let's give this a plural name since it is a list.

Also, let's either name it XPCSHELL_TEST_DIRS and define directories containing xpcshell.ini files. Or, let's name it XPCSHELL_TEST_MANIFESTS and have the value contain paths to (presumably) xpcshell.ini files. I prefer the latter since it's more explicit and allows funkiness like multiple manifest files in the same directory (should we ever go that route).

::: python/mozbuild/mozbuild/test/backend/data/xpcshell_manifest/moz.build
@@ +2,5 @@
> +# Any copyright is dedicated to the Public Domain.
> +# http://creativecommons.org/publicdomain/zero/1.0/
> +
> +XPCSHELL_MANIFEST = [ 'aa', 'bb' ]
> +XPCSHELL_MANIFEST += [ 'cc', 'dd' ]

Nit: Don't pad whitespace inside lists.
Attachment #739022 - Flags: review?(gps) → feedback+
(In reply to Gregory Szorc [:gps] from comment #25)
> Comment on attachment 739022 [details] [diff] [review]
> bug 844655: Port XPCSHELL_TESTS to moz.build as XPCSHELL_MANIFEST
> 
> Review of attachment 739022 [details] [diff] [review]:
> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +93,5 @@
> >              yield Exports(sandbox, exports)
> >  
> > +        manifest = sandbox.get('XPCSHELL_MANIFEST')
> > +        if manifest:
> > +            yield XpcshellManifest(sandbox, manifest)
> 
> I would emit a separate object for each entry in the list. They are
> independent, so they should not be treated as a unit, right?

There currently is no logic on the back end to make use of individual elements from an emitter.  Easy enough to add but probably overkill at this point if we are simply trying to migrate data in bulk.

The remaining nits/var renames/etc should be ready shortly after a try run.
Try job passed but will need to re-run with edits converting xpcshell_manifest from a verbatim/space delimited scalar into a list.

https://tbpl.mozilla.org/?tree=Try&rev=dc364f297026
Attachment #738633 - Attachment is obsolete: true
Comment on attachment 741886 [details] [diff] [review]
Port XPCSHELL_TESTS to moz.build as XPCSHELL_TESTS_MANIFESTS

patch morphing from conversion to enhancements.
r=mshal carried forward.

Rename XPCSHELL_MANIFESTS to XPCSHELL_TESTS_MANIFESTS.
Assigned content changed from directory list to path to xpcshell.ini file.
Attachment #739022 - Attachment is obsolete: true
Comment on attachment 741974 [details] [diff] [review]
Port XPCSHELL_TESTS to moz.build as XPCSHELL_TESTS_MANIFESTS

Nit/cleanups from the last patch.

Changed xpcshell emitter to return a series of objects rather than a space delimited scalar/passthrough value.

Content format changed from $dir to $dir/xpcshell.ini.  backend will extract dirname($val) to maintain existing XPCSHELL_TEST syntax.

Unit test updated to work with asynchronous/list content { gather, sort then compare }.

try job pending
Attachment #741974 - Flags: review?(gps)
Comment on attachment 741974 [details] [diff] [review]
Port XPCSHELL_TESTS to moz.build as XPCSHELL_TESTS_MANIFESTS

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

Looks good.

::: python/mozbuild/mozbuild/test/frontend/data/xpcshell_manifests/moz.build
@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +XPCSHELL_TESTS_MANIFESTS += [ 'foo/xpcshell.ini', 'bar/xpcshell.ini' ]
> +XPCSHELL_TESTS_MANIFESTS += [ 'tans/xpcshell.ini' ]

Nit: cuddle braces.

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py
@@ +185,5 @@
> +            'foo/xpcshell.ini',
> +            'tans/xpcshell.ini',
> +            ]
> +
> +        self.assertEqual(len(inis), len(iniByDir))

Nit: You don't need this check since it is covered by the next line.
Attachment #741974 - Flags: review?(gps) → review+
https://tbpl.mozilla.org/?tree=Try&rev=295b35a5e0fe

xp failures were due to timeouts.

nit fixes + rebase and will be on the way to inbound.
What is the DISABLED_XPCSHELL_TESTS in the Makefile.in? AFAICT that is useless cruft. It should be removed.
remote: added 2 changesets with 207 changes to 207 files
remote: You can view your changes at the following URLs:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e49215d92524
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/04d7289c00a0
(In reply to Gregory Szorc [:gps] from comment #34)
> What is the DISABLED_XPCSHELL_TESTS in the Makefile.in? AFAICT that is
> useless cruft. It should be removed.

Temporary addition -- the DISABLED_ prefix was used for minimal edits within Makefile.in to reduce the chance for update conflicts.  After the patch lands and is stable a 2nd patch will follow removing the assignments and cleaning up a few other lingering items with the conversion.
https://hg.mozilla.org/mozilla-central/rev/e49215d92524
https://hg.mozilla.org/mozilla-central/rev/04d7289c00a0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Blocks: 869359
(In reply to Joey Armstrong [:joey] from comment #10)
> The 'todo' here was added as a placeholder.  Land the bulk variable
> translations & logic.  Then revisit the creepie-crawlies as cleanup in a
> followup patch.

Was this ever done? I can't figure out how to enable the indexedDB/ipc tests now.
Link your xpcshell.ini file(s) to the build config by adding a XPCSHELL_TESTS_MANIFESTS entry to your moz.build file. e.g.

https://hg.mozilla.org/mozilla-central/file/1a2d9a04ffb2/services/healthreport/tests/moz.build
Flags: needinfo?(joey)
That isn't sufficient. We need to copy over all the tests from another directory. There was a special rule that did this before that no longer works.
Flags: needinfo?(gps)
(In reply to ben turner [:bent] (needinfo? encouraged) from comment #40)
> That isn't sufficient. We need to copy over all the tests from another
> directory. There was a special rule that did this before that no longer
> works.

Does https://ci.mozilla.org/job/mozilla-central-docs/Build_Documentation/test_manifests.html answer your question? If not, this might require special consideration from a build peer. File a new bug please.
Flags: needinfo?(gps)
Hm, 'support-files' may do what the old rule did... I'll experiment.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.