Bug 1313716 - Don't provide a blank subsuite as a default in the manifestparser. r=ahal draft
authorChris Manchester <cmanchester@mozilla.com>
Fri, 28 Oct 2016 11:07:21 -0700
changeset 431074 be7a2504af6853abb1bc532a058738f33d8dcbee
parent 431012 1561c917ee27c3ea04bd69467e5b8c7c08102f2a
child 535366 94f892cfeaa8ce7b4fe15fb3d589e37692ee4d11
push id34005
push userbmo:cmanchester@mozilla.com
push dateFri, 28 Oct 2016 20:12:58 +0000
reviewersahal
bugs1313716
milestone52.0a1
Bug 1313716 - Don't provide a blank subsuite as a default in the manifestparser. r=ahal This causes consumers managing defaults themselves to fail to find a default subsuite for tests, because the manifestparser will have provided a blank default value by the time they incorporate defaults into a test definition. This patch removes the provided defaults and updates a number of places assuming the 'subsuite' field is always present. MozReview-Commit-ID: 1jPy52VmEPr
testing/mach_commands.py
testing/mochitest/mach_commands.py
testing/mozbase/manifestparser/manifestparser/manifestparser.py
testing/mozbase/manifestparser/tests/test_convert_directory.py
testing/mozbase/manifestparser/tests/test_default_overrides.py
testing/mozbase/manifestparser/tests/test_manifestparser.py
--- a/testing/mach_commands.py
+++ b/testing/mach_commands.py
@@ -315,17 +315,17 @@ class Test(MachCommandBase):
                 res = self._mach_context.commands.dispatch(
                     suite['mach_command'], self._mach_context,
                     **suite['kwargs'])
                 if res:
                     status = res
 
         buckets = {}
         for test in run_tests:
-            key = (test['flavor'], test['subsuite'])
+            key = (test['flavor'], test.get('subsuite', ''))
             buckets.setdefault(key, []).append(test)
 
         for (flavor, subsuite), tests in sorted(buckets.items()):
             if flavor not in TEST_FLAVORS:
                 print(UNKNOWN_FLAVOR % flavor)
                 status = 1
                 continue
 
--- a/testing/mochitest/mach_commands.py
+++ b/testing/mochitest/mach_commands.py
@@ -373,27 +373,27 @@ class MachCommands(MachCommandBase):
 
         suites = defaultdict(list)
         unsupported = set()
         for test in tests:
             # Filter out non-mochitests and unsupported flavors.
             if test['flavor'] not in ALL_FLAVORS:
                 continue
 
-            key = (test['flavor'], test['subsuite'])
+            key = (test['flavor'], test.get('subsuite', ''))
             if test['flavor'] not in flavors:
                 unsupported.add(key)
                 continue
 
             if subsuite == 'default':
                 # "--subsuite default" means only run tests that don't have a subsuite
-                if test['subsuite']:
+                if test.get('subsuite'):
                     unsupported.add(key)
                     continue
-            elif subsuite and test['subsuite'] != subsuite:
+            elif subsuite and test.get('subsuite', '') != subsuite:
                 unsupported.add(key)
                 continue
 
             suites[key].append(test)
 
         # This is a hack to introduce an option in mach to not send
         # filtered tests to the mochitest harness. Mochitest harness will read
         # the master manifest in that case.
--- a/testing/mozbase/manifestparser/manifestparser/manifestparser.py
+++ b/testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ -147,20 +147,16 @@ class ManifestParser(object):
         sections = read_ini(fp=fp, variables=defaults, strict=self.strict,
                             handle_defaults=self._handle_defaults)
         self.manifest_defaults[filename] = defaults
 
         parent_section_found = False
 
         # get the tests
         for section, data in sections:
-            subsuite = ''
-            if 'subsuite' in data:
-                subsuite = data['subsuite']
-
             # In case of defaults only, no other section than parent: has to
             # be processed.
             if defaults_only and not section.startswith('parent:'):
                 continue
 
             # read the parent manifest if specified
             if section.startswith('parent:'):
                 parent_section_found = True
@@ -216,17 +212,16 @@ class ManifestParser(object):
                 # When the rootdir is unknown, the relpath needs to be left
                 # unchanged. We use an empty string as rootdir in that case,
                 # which leaves relpath unchanged after slicing.
                 if path.startswith(rootdir):
                     _relpath = path[len(rootdir):]
                 else:
                     _relpath = relpath(path, rootdir)
 
-            test['subsuite'] = subsuite
             test['path'] = path
             test['relpath'] = _relpath
 
             if parentmanifest is not None:
                 # If a test was included by a parent manifest we may need to
                 # indicate that in the test object for the sake of identifying
                 # a test, particularly in the case a test file is included by
                 # multiple manifests.
--- a/testing/mozbase/manifestparser/tests/test_convert_directory.py
+++ b/testing/mozbase/manifestparser/tests/test_convert_directory.py
@@ -55,26 +55,22 @@ class TestDirectoryConversion(unittest.T
         stub = self.create_stub()
         try:
             stub = stub.replace(os.path.sep, "/")
             self.assertTrue(os.path.exists(stub) and os.path.isdir(stub))
 
             # Make a manifest for it
             manifest = convert([stub])
             out_tmpl = """[%(stub)s/bar]
-subsuite = 
 
 [%(stub)s/fleem]
-subsuite = 
 
 [%(stub)s/foo]
-subsuite = 
 
 [%(stub)s/subdir/subfile]
-subsuite = 
 
 """  # noqa
             self.assertEqual(str(manifest), out_tmpl % dict(stub=stub))
         except:
             raise
         finally:
             shutil.rmtree(stub)  # cleanup
 
--- a/testing/mozbase/manifestparser/tests/test_default_overrides.py
+++ b/testing/mozbase/manifestparser/tests/test_default_overrides.py
@@ -2,16 +2,17 @@
 
 # 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/.
 
 import os
 import unittest
 from manifestparser import ManifestParser
+from manifestparser import combine_fields
 
 here = os.path.dirname(os.path.abspath(__file__))
 
 
 class TestDefaultSkipif(unittest.TestCase):
     """Tests applying a skip-if condition in [DEFAULT] and || with the value for the test"""
 
     def test_defaults(self):
@@ -88,10 +89,27 @@ class TestOmitDefaults(unittest.TestCase
         }
         for path, defaults in expected_defaults.items():
             self.assertIn(path, parser.manifest_defaults)
             actual_defaults = parser.manifest_defaults[path]
             for key, value in defaults.items():
                 self.assertIn(key, actual_defaults)
                 self.assertEqual(value, actual_defaults[key])
 
+
+class TestSubsuiteDefaults(unittest.TestCase):
+    """Test that subsuites are handled correctly when managing defaults
+    outside of the manifest parser."""
+    def test_subsuite_defaults(self):
+        manifest = os.path.join(here, 'default-subsuite.ini')
+        parser = ManifestParser(manifests=(manifest,), handle_defaults=False)
+        expected_subsuites = {
+            'test1': 'baz',
+            'test2': 'foo',
+        }
+        defaults = parser.manifest_defaults[manifest]
+        for test in parser.tests:
+            value = combine_fields(defaults, test)
+            self.assertEqual(expected_subsuites[value['name']],
+                             value['subsuite'])
+
 if __name__ == '__main__':
     unittest.main()
--- a/testing/mozbase/manifestparser/tests/test_manifestparser.py
+++ b/testing/mozbase/manifestparser/tests/test_manifestparser.py
@@ -109,22 +109,20 @@ class TestManifestParser(unittest.TestCa
 
         # Write the output to a manifest:
         buffer = StringIO()
         parser.write(fp=buffer, global_kwargs={'foo': 'bar'})
         expected_output = """[DEFAULT]
 foo = bar
 
 [fleem]
-subsuite = 
 
 [include/flowers]
 blue = ocean
 red = roses
-subsuite = 
 yellow = submarine"""  # noqa
 
         self.assertEqual(buffer.getvalue().strip(),
                          expected_output)
 
     def test_invalid_path(self):
         """
         Test invalid path should not throw when not strict
@@ -152,17 +150,17 @@ yellow = submarine"""  # noqa
         # DEFAULT values should be the ones from level 1
         self.assertEqual(parser.get('name', x='level_1'),
                          ['test_3'])
 
         # Write the output to a manifest:
         buffer = StringIO()
         parser.write(fp=buffer, global_kwargs={'x': 'level_1'})
         self.assertEqual(buffer.getvalue().strip(),
-                         '[DEFAULT]\nx = level_1\n\n[test_3]\nsubsuite =')
+                         '[DEFAULT]\nx = level_1\n\n[test_3]')
 
     def test_parent_defaults(self):
         """
         Test downstream variables should overwrite upstream variables
         """
         parent_example = os.path.join(here, 'parent', 'level_1', 'level_2',
                                       'level_3', 'level_3_default.ini')
         parser = ManifestParser(manifests=(parent_example,))
@@ -177,17 +175,17 @@ yellow = submarine"""  # noqa
         # DEFAULT values should be the ones from level 3
         self.assertEqual(parser.get('name', x='level_3'),
                          ['test_3'])
 
         # Write the output to a manifest:
         buffer = StringIO()
         parser.write(fp=buffer, global_kwargs={'x': 'level_3'})
         self.assertEqual(buffer.getvalue().strip(),
-                         '[DEFAULT]\nx = level_3\n\n[test_3]\nsubsuite =')
+                         '[DEFAULT]\nx = level_3\n\n[test_3]')
 
     def test_parent_defaults_include(self):
         parent_example = os.path.join(here, 'parent', 'include', 'manifest.ini')
         parser = ManifestParser(manifests=(parent_example,))
 
         # global defaults should inherit all includes
         self.assertEqual(parser.get('name', top='data'),
                          ['testFirst.js', 'testSecond.js'])