From d4d66f19dce8dbe7ee17626982f4220296034ef4 Mon Sep 17 00:00:00 2001 From: James Graham Date: Fri, 9 Aug 2019 17:48:53 +0000 Subject: [PATCH] Bug 1571755 - Add a command for merging wpt metadata, r=maja_zf This command is inteded to be usable as a git mergetool for the specific case of merging ini files in a way that should always succeed, produces reasonably likely output, but isn't reliably correct. The main use case is for the sync bot where we update metadata on branches and experience conflicts when we also have changes on master. We don't necessarily need to resolve these perfectly but we do need to provide a resolution automatically since otherwise it blocks syncs. Since the ini parser is in-tree we want to make this an in-tree command that the sync can use to provide the resolution. The general strategy is that we want to prefer the "new" metadata where possible. This won't always be correct e.g. if a test got fixed on master and simultaneously got edited to go from TIMEOUT to FAIL in a browser without the fix. But it's not a bad approximation (and generally a human will struggle to do the merge better by hand, so we have to assume later try jobs will fix things). Differential Revision: https://phabricator.services.mozilla.com/D40832 --HG-- extra : moz-landing-system : lando --- testing/web-platform/mach_commands.py | 13 + testing/web-platform/metamerge.py | 275 ++++++++++++++++++ testing/web-platform/moz.build | 2 + testing/web-platform/python.ini | 3 + testing/web-platform/test_metamerge.py | 190 ++++++++++++ .../wptrunner/wptmanifest/backends/base.py | 2 +- 6 files changed, 484 insertions(+), 1 deletion(-) create mode 100644 testing/web-platform/metamerge.py create mode 100644 testing/web-platform/python.ini create mode 100644 testing/web-platform/test_metamerge.py diff --git a/testing/web-platform/mach_commands.py b/testing/web-platform/mach_commands.py index 69500a0218c5..ce50b264d6a5 100644 --- a/testing/web-platform/mach_commands.py +++ b/testing/web-platform/mach_commands.py @@ -252,6 +252,10 @@ def create_parser_metadata_summary(): return metasummary.create_parser() +def create_parser_metadata_merge(): + import metamerge + return metamerge.get_parser() + def create_parser_serve(): sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "tests", "tools"))) @@ -365,6 +369,15 @@ class MachCommands(MachCommandBase): wpt_setup = self._spawn(WebPlatformTestsRunnerSetup) return metasummary.run(wpt_setup.topsrcdir, wpt_setup.topobjdir, **params) + @Command("wpt-metadata-merge", + category="testing", + parser=create_parser_metadata_merge) + def wpt_meta_merge(self, **params): + import metamerge + if params["dest"] is None: + params["dest"] = params["current"] + return metamerge.run(**params) + @Command("wpt-unittest", category="testing", description="Run the wpt tools and wptrunner unit tests", diff --git a/testing/web-platform/metamerge.py b/testing/web-platform/metamerge.py new file mode 100644 index 000000000000..e48b5413151a --- /dev/null +++ b/testing/web-platform/metamerge.py @@ -0,0 +1,275 @@ +import argparse +import logging +import os +from collections import defaultdict, namedtuple + +from wptrunner.wptmanifest.serializer import serialize +from wptrunner.wptmanifest.backends import base +from wptrunner.wptmanifest.node import KeyValueNode + +here = os.path.dirname(__file__) +logger = logging.getLogger(__name__) + + +class Compiler(base.Compiler): + def visit_KeyValueNode(self, node): + key_name = node.data + values = [] + for child in node.children: + values.append(self.visit(child)) + + self.output_node.set(key_name, values) + + def visit_ConditionalNode(self, node): + assert len(node.children) == 2 + # For conditional nodes, just return the subtree + return serialize(node.children[0]), self.visit(node.children[1]) + + def visit_UnaryExpressionNode(self, node): + raise NotImplementedError + + def visit_BinaryExpressionNode(self, node): + raise NotImplementedError + + def visit_UnaryOperatorNode(self, node): + raise NotImplementedError + + def visit_BinaryOperatorNode(self, node): + raise NotImplementedError + + +class ExpectedManifest(base.ManifestItem): + def __init__(self, node): + """Object representing all the tests in a particular manifest""" + base.ManifestItem.__init__(self, node) + self.child_map = {} + + def append(self, child): + """Add a test to the manifest""" + base.ManifestItem.append(self, child) + self.child_map[child.name] = child + + def insert(self, child): + self.append(child) + self.node.append(child.node) + + def delete(self, child): + del self.child_map[child.name] + child.node.remove() + child.remove() + + +class TestManifestItem(ExpectedManifest): + def set_expected(self, other_manifest): + for item in self.node.children: + if isinstance(item, KeyValueNode) and item.data == "expected": + assert "expected" in self._data + item.remove() + del self._data["expected"] + break + + for item in other_manifest.node.children: + if isinstance(item, KeyValueNode) and item.data == "expected": + assert "expected" in other_manifest._data + item.remove() + self.node.children.append(item) + self._data["expected"] = other_manifest._data.pop("expected") + break + + +def data_cls_getter(output_node, visited_node): + # visited_node is intentionally unused + if output_node is None: + return ExpectedManifest + if isinstance(output_node, ExpectedManifest): + return TestManifestItem + raise ValueError + + +def compile(stream, data_cls_getter=None, **kwargs): + return base.compile(Compiler, + stream, + data_cls_getter=data_cls_getter, + **kwargs) + + +def get_manifest(manifest_path): + """Get the ExpectedManifest for a particular manifest path""" + try: + with open(manifest_path) as f: + return compile(f, + data_cls_getter=data_cls_getter) + except IOError: + return None + + +def indent(str_data, indent=2): + rv = [] + for line in str_data.splitlines(): + rv.append("%s%s" % (" " * indent, line)) + return "\n".join(rv) + + +class Differences(object): + def __init__(self): + self.added = [] + self.deleted = [] + self.modified = [] + + def __nonzero__(self): + return bool(self.added or self.deleted or self.modified) + + def __str__(self): + modified = [] + for item in self.modified: + if isinstance(item, TestModified): + modified.append(" %s\n %s\n%s" % (item[0], item[1], indent(str(item[2]), 4))) + else: + assert isinstance(item, ExpectedModified) + modified.append(" %s\n %s %s" % item) + return "Added:\n%s\nDeleted:\n%s\nModified:\n%s\n" % ( + "\n".join(" %s:\n %s" % item for item in self.added), + "\n".join(" %s" % item for item in self.deleted), + "\n".join(modified)) + + +TestModified = namedtuple("TestModified", ["test", "test_manifest", "differences"]) + + +ExpectedModified = namedtuple("ExpectedModified", ["test", "ancestor_manifest", "new_manifest"]) + + +def compare_test(test, ancestor_manifest, new_manifest): + changes = Differences() + + compare_expected(changes, None, ancestor_manifest, new_manifest) + + for subtest, ancestor_subtest_manifest in ancestor_manifest.child_map.iteritems(): + compare_expected(changes, subtest, ancestor_subtest_manifest, + new_manifest.child_map.get(subtest)) + + for subtest, subtest_manifest in new_manifest.child_map.iteritems(): + if subtest not in ancestor_manifest.child_map: + changes.added.append((subtest, subtest_manifest)) + + return changes + + +def compare_expected(changes, subtest, ancestor_manifest, new_manifest): + if (not (ancestor_manifest and ancestor_manifest.has_key("expected")) and + (new_manifest and new_manifest.has_key("expected"))): + changes.added.append((subtest, new_manifest)) + elif (ancestor_manifest and ancestor_manifest.has_key("expected") and + not (new_manifest and new_manifest.has_key("expected"))): + changes.deleted.append(subtest) + elif (ancestor_manifest and ancestor_manifest.has_key("expected") and + new_manifest and new_manifest.has_key("expected")): + old_expected = ancestor_manifest.get("expected") + new_expected = new_manifest.get("expected") + if expected_values_changed(old_expected, new_expected): + changes.modified.append(ExpectedModified(subtest, ancestor_manifest, new_manifest)) + + +def expected_values_changed(old_expected, new_expected): + if len(old_expected) != len(new_expected): + return True + + old_dict = {} + new_dict = {} + for dest, cond_lines in [(old_dict, old_expected), (new_dict, new_expected)]: + for cond_line in cond_lines: + if isinstance(cond_line, tuple): + condition, value = cond_line + else: + condition = None + value = cond_line + dest[condition] = value + + return new_dict != old_dict + + +def record_changes(ancestor_manifest, new_manifest): + changes = Differences() + + for test, test_manifest in new_manifest.child_map.iteritems(): + if test not in ancestor_manifest.child_map: + changes.added.append((test, test_manifest)) + else: + ancestor_test_manifest = ancestor_manifest.child_map[test] + test_differences = compare_test(test, + ancestor_test_manifest, + test_manifest) + if test_differences: + changes.modified.append(TestModified(test, test_manifest, test_differences)) + + for test, test_manifest in ancestor_manifest.child_map.iteritems(): + if test not in new_manifest.child_map: + changes.deleted.append(test) + + return changes + + +def apply_changes(current_manifest, changes): + for test, test_manifest in changes.added: + if test in current_manifest.child_map: + current_manifest.delete(current_manifest.child_map[test]) + current_manifest.insert(test_manifest) + + for test in changes.deleted: + if test in current_manifest.child_map: + current_manifest.delete(current_manifest.child_map[test]) + + for item in changes.modified: + if isinstance(item, TestModified): + test, new_manifest, test_changes = item + if test in current_manifest.child_map: + apply_changes(current_manifest.child_map[test], test_changes) + else: + current_manifest.insert(new_manifest) + else: + assert isinstance(item, ExpectedModified) + subtest, ancestor_manifest, new_manifest = item + if not subtest: + current_manifest.set_expected(new_manifest) + elif subtest in current_manifest.child_map: + current_manifest.child_map[subtest].set_expected(new_manifest) + else: + current_manifest.insert(new_manifest) + + +def get_parser(): + parser = argparse.ArgumentParser() + parser.add_argument("ancestor") + parser.add_argument("current") + parser.add_argument("new") + parser.add_argument("dest", nargs="?") + return parser + + +def get_parser_mergetool(): + parser = argparse.ArgumentParser() + parser.add_argument("--no-overwrite", dest="overwrite", action="store_false") + return parser + + + +def make_changes(ancestor_manifest, current_manifest, new_manifest): + changes = record_changes(ancestor_manifest, new_manifest) + apply_changes(current_manifest, changes) + + return serialize(current_manifest.node) + + +def run(ancestor, current, new, dest): + ancestor_manifest = get_manifest(ancestor) + current_manifest = get_manifest(current) + new_manifest = get_manifest(new) + + + updated_current_str = make_changes(ancestor_manifest, current_manifest, new_manifest) + + if dest != "-": + with open(dest, "wb") as f: + f.write(updated_current_str) + else: + print(updated_current_str) diff --git a/testing/web-platform/moz.build b/testing/web-platform/moz.build index b015c1ad9a23..0a5a88be47e1 100644 --- a/testing/web-platform/moz.build +++ b/testing/web-platform/moz.build @@ -16,6 +16,8 @@ TEST_HARNESS_FILES['web-platform'].prefs += [ '/build/sanitizers/lsan_suppressions.txt', ] +PYTHON_UNITTEST_MANIFESTS += ['python.ini'] + with Files("**"): SCHEDULES.exclusive = [ 'web-platform-tests', diff --git a/testing/web-platform/python.ini b/testing/web-platform/python.ini new file mode 100644 index 000000000000..7b6290804f2b --- /dev/null +++ b/testing/web-platform/python.ini @@ -0,0 +1,3 @@ +[DEFAULT] +subsuite = mozbase +[test_metamerge.py] diff --git a/testing/web-platform/test_metamerge.py b/testing/web-platform/test_metamerge.py new file mode 100644 index 000000000000..c4fff8fb048e --- /dev/null +++ b/testing/web-platform/test_metamerge.py @@ -0,0 +1,190 @@ +from io import BytesIO + +import mozunit + +import metamerge + +ancestor = """ +global-new-deleted: A +global-new-changed: A +global-current-deleted: A +global-current-changed: A + +[failing-test.html] + [Unchanged subtest] + expected: FAIL + + [New deleted subtest] + expected: FAIL + + [New modified subtest] + expected: TIMEOUT + + [Current deleted subtest] + expected: FAIL + + [New modified current deleted] + expected: FAIL + + [Ancestor no expected new expected] + bug: 1234 + +[new-deleted-test.html] + [Deleted subtest] + expected: FAIL + +[current-deleted-test.html] + [Deleted subtest] + expected: FAIL + +[test-modified.html] + expected: TIMEOUT + +[new-modified-current-deleted.html] + expected: + if os == "linux": FAIL + TIMEOUT +""" + +new = """ +global-new-added: A +global-new-changed: B +global-current-deleted: A +global-current-changed: A + +[failing-test.html] + [Unchanged subtest] + expected: FAIL + + [New added subtest] + expected: FAIL + + [New modified subtest] + expected: + if os == "linux": FAIL + TIMEOUT + + [Current deleted subtest] + expected: FAIL + + [New modified current deleted] + expected: TIMEOUT + + [Ancestor no expected new expected] + bug: 1234 + expected: FAIL + +[new-added-test.html] + [Added subtest] + expected: FAIL + +[current-deleted-test.html] + [Deleted subtest] + expected: FAIL + +[test-modified.html] + expected: + if os == "linux": FAIL + +[new-modified-current-deleted.html] + expected: + if os == "linux": FAIL + if os == "mac": FAIL + TIMEOUT +""" + +current = """ +global-new-deleted: A +global-new-changed: A +global-current-added: A +global-current-changed: B + +[failing-test.html] + [Unchanged subtest] + expected: FAIL + + [New deleted subtest] + expected: FAIL + + [New modified subtest] + expected: TIMEOUT + + [Current added subtest] + expected: FAIL + + [Ancestor no expected new expected] + bug: 1234 + +[new-deleted-test.html] + [Deleted subtest] + expected: FAIL + +[current-added-test.html] + [Added subtest] + expected: FAIL +""" + +updated = """global-new-deleted: A +global-new-changed: A +global-current-added: A +global-current-changed: B +[failing-test.html] + [Unchanged subtest] + expected: FAIL + + [New modified subtest] + expected: + if os == "linux": FAIL + TIMEOUT + + [Current added subtest] + expected: FAIL + + [Ancestor no expected new expected] + bug: 1234 + expected: FAIL + + [New added subtest] + expected: FAIL + + [New modified current deleted] + expected: TIMEOUT + + +[current-added-test.html] + [Added subtest] + expected: FAIL + + +[new-added-test.html] + [Added subtest] + expected: FAIL + + +[new-modified-current-deleted.html] + expected: + if os == "linux": FAIL + if os == "mac": FAIL + TIMEOUT + +[test-modified.html] + expected: + if os == "linux": FAIL +""" + + +def test_merge(): + def get_manifest(str_data): + bytes_io = BytesIO(str_data) + return metamerge.compile(bytes_io, + metamerge.data_cls_getter) + ancestor_manifest = get_manifest(ancestor) + current_manifest = get_manifest(current) + new_manifest = get_manifest(new) + + assert metamerge.make_changes(ancestor_manifest, + current_manifest, + new_manifest) == updated + +if __name__ == '__main__': + mozunit.main() diff --git a/testing/web-platform/tests/tools/wptrunner/wptrunner/wptmanifest/backends/base.py b/testing/web-platform/tests/tools/wptrunner/wptrunner/wptmanifest/backends/base.py index d2b23b4b58bf..45e2147fc4ca 100644 --- a/testing/web-platform/tests/tools/wptrunner/wptrunner/wptmanifest/backends/base.py +++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/wptmanifest/backends/base.py @@ -169,7 +169,7 @@ class ManifestItem(object): def remove(self): if self.parent: - self.parent.children.remove(child) + self.parent.children.remove(self) self.parent = None def iterchildren(self, name=None):