Bug 1498636 - Separate "include" variables from manifest defaults. r=ahal, a=test-only
authorRob Wu <rob@robwu.nl>
Thu, 07 Feb 2019 15:10:25 +0000
changeset 515833 fc9047475850942b21cec35ed83c6f09666fa512
parent 515832 9b3d629cf829c77442955d7efd9101a73511f187
child 515834 bc23b491541e589b832b4605f188c22260f0f957
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersahal, test-only
bugs1498636
milestone66.0
Bug 1498636 - Separate "include" variables from manifest defaults. r=ahal, a=test-only Test manifests may be included by multiple other manifests, optionally with additional variables below the `[include:...]` section header. These additional variables are specific to the manifest that contained the "include" section, and should not inadvertently be shared with other manifests that also happen to include this manifest. To achieve that, store the defaults for included manifests in a (path to parent manifest, path to included manifest) tuple instead of just the included manifest. Differential Revision: https://phabricator.services.mozilla.com/D18086
testing/mozbase/manifestparser/manifestparser/manifestparser.py
testing/mozbase/manifestparser/tests/test_manifestparser.py
testing/mozbase/moztest/moztest/resolve.py
--- a/testing/mozbase/manifestparser/manifestparser/manifestparser.py
+++ b/testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ -145,17 +145,30 @@ class ManifestParser(object):
             rootdir = ""
         else:
             assert os.path.isabs(self.rootdir)
             rootdir = self.rootdir + os.path.sep
 
         # read the configuration
         sections = read_ini(fp=fp, variables=defaults, strict=self.strict,
                             handle_defaults=self._handle_defaults)
-        self.manifest_defaults[filename] = defaults
+        if parentmanifest and filename:
+            # A manifest can be read multiple times, via "include:", optionally
+            # with section-specific variables. These variables only apply to
+            # the included manifest when included via the same parent manifest,
+            # so they must be associated with (parentmanifest, filename).
+            #
+            # |defaults| is a combination of variables, in the following order:
+            # - The defaults of the ancestor manifests if self._handle_defaults
+            #   is True.
+            # - Any variables from the "[include:...]" section.
+            # - The defaults of the included manifest.
+            self.manifest_defaults[(parentmanifest, filename)] = defaults
+        else:
+            self.manifest_defaults[filename] = defaults
 
         parent_section_found = False
 
         # get the tests
         for section, data in sections:
             # In case of defaults only, no other section than parent: has to
             # be processed.
             if defaults_only and not section.startswith('parent:'):
@@ -336,20 +349,27 @@ class ManifestParser(object):
             return [test[_key] for test in tests]
 
         # return the tests
         return tests
 
     def manifests(self, tests=None):
         """
         return manifests in order in which they appear in the tests
+        If |tests| is not set, the order of the manifests is unspecified.
         """
         if tests is None:
+            manifests = []
             # Make sure to return all the manifests, even ones without tests.
-            return self.manifest_defaults.keys()
+            for manifest in self.manifest_defaults.keys():
+                if isinstance(manifest, tuple):
+                    parentmanifest, manifest = manifest
+                if manifest not in manifests:
+                    manifests.append(manifest)
+            return manifests
 
         manifests = []
         for test in tests:
             manifest = test.get('manifest')
             if not manifest:
                 continue
             if manifest not in manifests:
                 manifests.append(manifest)
--- a/testing/mozbase/manifestparser/tests/test_manifestparser.py
+++ b/testing/mozbase/manifestparser/tests/test_manifestparser.py
@@ -123,16 +123,157 @@ foo = bar
 [include/flowers]
 blue = ocean
 red = roses
 yellow = submarine"""  # noqa
 
         self.assertEqual(buffer.getvalue().strip(),
                          expected_output)
 
+    def test_include_manifest_defaults(self):
+        """
+        Test that manifest_defaults and manifests() are correctly populated
+        when includes are used.
+        """
+
+        include_example = os.path.join(here, 'include-example.ini')
+        noinclude_example = os.path.join(here, 'just-defaults.ini')
+        bar_path = os.path.join(here, 'include', 'bar.ini')
+        foo_path = os.path.join(here, 'include', 'foo.ini')
+
+        parser = ManifestParser(manifests=(include_example, noinclude_example))
+
+        # Standalone manifests must be appear as-is.
+        self.assertTrue(include_example in parser.manifest_defaults)
+        self.assertTrue(noinclude_example in parser.manifest_defaults)
+
+        # Included manifests must only appear together with the parent manifest
+        # that included the manifest.
+        self.assertFalse(bar_path in parser.manifest_defaults)
+        self.assertFalse(foo_path in parser.manifest_defaults)
+        self.assertTrue((include_example, bar_path) in parser.manifest_defaults)
+        self.assertTrue((include_example, foo_path) in parser.manifest_defaults)
+
+        # manifests() must only return file paths (strings).
+        manifests = parser.manifests()
+        self.assertEqual(len(manifests), 4)
+        self.assertIn(foo_path, manifests)
+        self.assertIn(bar_path, manifests)
+        self.assertIn(include_example, manifests)
+        self.assertIn(noinclude_example, manifests)
+
+    def test_include_handle_defaults_False(self):
+        """
+        Test that manifest_defaults and manifests() are correct even when
+        handle_defaults is set to False.
+        """
+        manifest = os.path.join(here, 'include-example.ini')
+        foo_path = os.path.join(here, 'include', 'foo.ini')
+
+        parser = ManifestParser(manifests=(manifest,), handle_defaults=False)
+
+        self.assertIn(manifest, parser.manifest_defaults)
+        self.assertNotIn(foo_path, parser.manifest_defaults)
+        self.assertIn((manifest, foo_path), parser.manifest_defaults)
+        self.assertEqual(parser.manifest_defaults[manifest],
+                         {
+                             'foo': 'bar',
+                             'here': here,
+                         })
+        self.assertEqual(parser.manifest_defaults[(manifest, foo_path)],
+                         {
+                             'here': os.path.join(here, 'include'),
+                             'red': 'roses',
+                             'blue': 'ocean',
+                             'yellow': 'daffodils',
+                         })
+
+    def test_include_repeated(self):
+        """
+        Test that repeatedly included manifests are independent of each other.
+        """
+        include_example = os.path.join(here, 'include-example.ini')
+        included_foo = os.path.join(here, 'include', 'foo.ini')
+
+        # In the expected output, blue and yellow have the values from foo.ini
+        # (ocean, submarine) instead of the ones from include-example.ini
+        # (violets, daffodils), because the defaults in the included file take
+        # precedence over the values from the parent.
+        include_output = """[include/crash-handling]
+foo = fleem
+
+[fleem]
+foo = bar
+
+[include/flowers]
+blue = ocean
+foo = bar
+red = roses
+yellow = submarine
+
+"""
+        included_output = """[include/flowers]
+blue = ocean
+yellow = submarine
+
+"""
+
+        parser = ManifestParser(manifests=(include_example, included_foo),
+                                rootdir=here)
+        self.assertEqual(parser.get('name'),
+                         ['crash-handling', 'fleem', 'flowers', 'flowers'])
+        self.assertEqual([(test['name'], os.path.basename(test['manifest']))
+                          for test in parser.tests],
+                         [('crash-handling', 'bar.ini'),
+                          ('fleem', 'include-example.ini'),
+                          ('flowers', 'foo.ini'),
+                          ('flowers', 'foo.ini')])
+        self.check_included_repeat(parser, parser.tests[3], parser.tests[2],
+                                   "%s%s" % (include_output, included_output))
+
+        # Same tests, but with the load order of the manifests swapped.
+        parser = ManifestParser(manifests=(included_foo, include_example),
+                                rootdir=here)
+        self.assertEqual(parser.get('name'),
+                         ['flowers', 'crash-handling', 'fleem', 'flowers'])
+        self.assertEqual([(test['name'], os.path.basename(test['manifest']))
+                          for test in parser.tests],
+                         [('flowers', 'foo.ini'),
+                          ('crash-handling', 'bar.ini'),
+                          ('fleem', 'include-example.ini'),
+                          ('flowers', 'foo.ini')])
+        self.check_included_repeat(parser, parser.tests[0], parser.tests[3],
+                                   "%s%s" % (included_output, include_output))
+
+    def check_included_repeat(self, parser, isolated_test, included_test,
+                              expected_output):
+        include_example = os.path.join(here, 'include-example.ini')
+        included_foo = os.path.join(here, 'include', 'foo.ini')
+        manifest_default_key = (include_example, included_foo)
+
+        self.assertFalse('ancestor-manifest' in isolated_test)
+        self.assertEqual(included_test['ancestor-manifest'],
+                         os.path.join(here, 'include-example.ini'))
+
+        self.assertTrue(include_example in parser.manifest_defaults)
+        self.assertTrue(included_foo in parser.manifest_defaults)
+        self.assertTrue(manifest_default_key in parser.manifest_defaults)
+        self.assertEqual(parser.manifest_defaults[manifest_default_key],
+                         {
+                             'foo': 'bar',
+                             'here': os.path.join(here, 'include'),
+                             'red': 'roses',
+                             'blue': 'ocean',
+                             'yellow': 'daffodils',
+                         })
+
+        buffer = StringIO()
+        parser.write(fp=buffer)
+        self.assertEqual(buffer.getvalue(), expected_output)
+
     def test_invalid_path(self):
         """
         Test invalid path should not throw when not strict
         """
         manifest = os.path.join(here, 'include-invalid.ini')
         ManifestParser(manifests=(manifest,), strict=False)
 
     def test_parent_inheritance(self):
--- a/testing/mozbase/moztest/moztest/resolve.py
+++ b/testing/mozbase/moztest/moztest/resolve.py
@@ -326,16 +326,20 @@ class TestMetadata(object):
                 defaults = pickle.load(fh)
         for path, tests in test_data.items():
             for metadata in tests:
                 if defaults:
                     defaults_manifests = [metadata['manifest']]
 
                     ancestor_manifest = metadata.get('ancestor-manifest')
                     if ancestor_manifest:
+                        # The (ancestor manifest, included manifest) tuple
+                        # contains the defaults of the included manifest, so
+                        # use it instead of [metadata['manifest']].
+                        defaults_manifests[0] = (ancestor_manifest, metadata['manifest'])
                         defaults_manifests.append(ancestor_manifest)
 
                     for manifest in defaults_manifests:
                         manifest_defaults = defaults.get(manifest)
                         if manifest_defaults:
                             metadata = manifestparser.combine_fields(manifest_defaults,
                                                                      metadata)
                 self._tests_by_path[path].append(metadata)