From 735621971dbd27c0e7fc52ef3d37fcf6a8de19ec Mon Sep 17 00:00:00 2001 From: John Chilton Date: Thu, 15 Oct 2020 15:23:16 -0400 Subject: [PATCH 1/2] Rework new collection order testing in 20.09. This was added in 20.09 with https://github.com/galaxyproject/galaxy/pull/9684/files. There were some things I didn't love that I think are corrected here. - Preserve legacy behavior of not enforcing sort order but make it now contigent on profile being 20.09 or newer. - Run all the element tests and then do order checking for a more specific error message: ``galaxy.tool_util.verify.interactor.JobOutputsError: Collection identifier '1' found out of order, expected order (['2', '1', '10']) of the tool generated collection elements ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10']`` - Add test case for older default based on new test case. - Fix the new attribute that is used to check the order so that it isn't required (this broke Planemo workflow testing) and so the name doesn't clash with an existing element attribute on collection elements in the API/model. --- lib/galaxy/tool_util/parser/xml.py | 29 +++++----- lib/galaxy/tool_util/verify/interactor.py | 54 +++++++++++-------- test/functional/tools/discover_sort_by.xml | 2 +- .../tools/discover_sort_by_legacy_test.xml | 42 +++++++++++++++ 4 files changed, 90 insertions(+), 37 deletions(-) create mode 100644 test/functional/tools/discover_sort_by_legacy_test.xml diff --git a/lib/galaxy/tool_util/parser/xml.py b/lib/galaxy/tool_util/parser/xml.py index 297ce801c15..88206c1ec09 100644 --- a/lib/galaxy/tool_util/parser/xml.py +++ b/lib/galaxy/tool_util/parser/xml.py @@ -513,7 +513,8 @@ class XmlToolSource(ToolSource): if tests_elem is not None: for i, test_elem in enumerate(tests_elem.findall("test")): - tests.append(_test_elem_to_dict(test_elem, i)) + profile = self.parse_profile() + tests.append(_test_elem_to_dict(test_elem, i, profile)) return rval @@ -533,10 +534,10 @@ class XmlToolSource(ToolSource): return python_template_version -def _test_elem_to_dict(test_elem, i): +def _test_elem_to_dict(test_elem, i, profile=None): rval = dict( outputs=__parse_output_elems(test_elem), - output_collections=__parse_output_collection_elems(test_elem), + output_collections=__parse_output_collection_elems(test_elem, profile=profile), inputs=__parse_input_elems(test_elem, i), expect_num_outputs=test_elem.get("expect_num_outputs"), command=__parse_assert_list_from_elem(test_elem.find("assert_command")), @@ -579,36 +580,38 @@ def __parse_command_elem(test_elem): return __parse_assert_list_from_elem(assert_elem) -def __parse_output_collection_elems(test_elem): +def __parse_output_collection_elems(test_elem, profile=None): output_collections = [] for output_collection_elem in test_elem.findall("output_collection"): - output_collection_def = __parse_output_collection_elem(output_collection_elem) + output_collection_def = __parse_output_collection_elem(output_collection_elem, profile=profile) output_collections.append(output_collection_def) return output_collections -def __parse_output_collection_elem(output_collection_elem): +def __parse_output_collection_elem(output_collection_elem, profile=None): attrib = dict(output_collection_elem.attrib) name = attrib.pop('name', None) if name is None: raise Exception("Test output collection does not have a 'name'") - element_tests = __parse_element_tests(output_collection_elem) + element_tests = __parse_element_tests(output_collection_elem, profile=profile) return TestCollectionOutputDef(name, attrib, element_tests).to_dict() -def __parse_element_tests(parent_element): - element_tests = {} +def __parse_element_tests(parent_element, profile=None): + element_tests = OrderedDict() for idx, element in enumerate(parent_element.findall("element")): element_attrib = dict(element.attrib) identifier = element_attrib.pop('name', None) if identifier is None: raise Exception("Test primary dataset does not have a 'identifier'") - element_tests[identifier] = __parse_test_attributes(element, element_attrib, parse_elements=True) - element_tests[identifier][1]["element_index"] = idx + element_tests[identifier] = __parse_test_attributes(element, element_attrib, parse_elements=True, profile=profile) + if profile and profile >= "20.09": + element_tests[identifier][1]["expected_sort_order"] = idx + return element_tests -def __parse_test_attributes(output_elem, attrib, parse_elements=False, parse_discovered_datasets=False): +def __parse_test_attributes(output_elem, attrib, parse_elements=False, parse_discovered_datasets=False, profile=None): assert_list = __parse_assert_list(output_elem) # Allow either file or value to specify a target file to compare result with @@ -638,7 +641,7 @@ def __parse_test_attributes(output_elem, attrib, parse_elements=False, parse_dis checksum = attrib.get("checksum", None) element_tests = {} if parse_elements: - element_tests = __parse_element_tests(output_elem) + element_tests = __parse_element_tests(output_elem, profile=profile) primary_datasets = {} if parse_discovered_datasets: diff --git a/lib/galaxy/tool_util/verify/interactor.py b/lib/galaxy/tool_util/verify/interactor.py index 62eed869d7c..6801b41d4ae 100644 --- a/lib/galaxy/tool_util/verify/interactor.py +++ b/lib/galaxy/tool_util/verify/interactor.py @@ -724,36 +724,29 @@ def verify_collection(output_collection_def, data_collection, verify_dataset): message = template % (name, expected_element_count, actual_element_count) raise AssertionError(message) + def get_element(elements, id): + for element in elements: + if element["element_identifier"] == id: + return element + return False + def verify_elements(element_objects, element_tests): - sorted_test_ids = [None] * len(element_tests) + # sorted_test_ids = [None] * len(element_tests) + expected_sort_order = [] + + eo_ids = [_["element_identifier"] for _ in element_objects] for element_identifier, element_test in element_tests.items(): if isinstance(element_test, dict): element_outfile, element_attrib = None, element_test else: element_outfile, element_attrib = element_test - sorted_test_ids[element_attrib["element_index"]] = element_identifier + if 'expected_sort_order' in element_attrib: + expected_sort_order.append(element_identifier) - i = 0 - for element_identifier in sorted_test_ids: - element_test = element_tests[element_identifier] - if isinstance(element_test, dict): - element_outfile, element_attrib = None, element_test - else: - element_outfile, element_attrib = element_test - - element = None - while i < len(element_objects): - if element_objects[i]["element_identifier"] == element_identifier: - element = element_objects[i] - i += 1 - break - i += 1 - - if element is None: - template = "Failed to find identifier '%s' of test collection %s in the tool generated collection elements %s (at the correct position)" - eo_ids = [_["element_identifier"] for _ in element_objects] - message = template % (element_identifier, sorted_test_ids, - eo_ids) + element = get_element(element_objects, element_identifier) + if not element: + template = "Failed to find identifier '%s' in the tool generated collection elements %s" + message = template % (element_identifier, eo_ids) raise AssertionError(message) element_type = element["element_type"] @@ -763,6 +756,21 @@ def verify_collection(output_collection_def, data_collection, verify_dataset): elements = element["object"]["elements"] verify_elements(elements, element_attrib.get("elements", {})) + if len(expected_sort_order) > 0: + i = 0 + for element_identifier in expected_sort_order: + element = None + while i < len(element_objects): + if element_objects[i]["element_identifier"] == element_identifier: + element = element_objects[i] + i += 1 + break + i += 1 + if element is None: + template = "Collection identifier '%s' found out of order, expected order of %s for the tool generated collection elements %s" + message = template % (element_identifier, expected_sort_order, eo_ids) + raise AssertionError(message) + verify_elements(data_collection["elements"], output_collection_def.element_tests) diff --git a/test/functional/tools/discover_sort_by.xml b/test/functional/tools/discover_sort_by.xml index 5e46342253a..cb5c1949673 100644 --- a/test/functional/tools/discover_sort_by.xml +++ b/test/functional/tools/discover_sort_by.xml @@ -1,4 +1,4 @@ - + + + \$i.txt; +done +]]> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 4c3932df60f687502e7b6c6d38aa0e44d46d6f98 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Fri, 16 Oct 2020 07:56:06 -0400 Subject: [PATCH 2/2] Improvements to #10434 based on comments from @bernt-matthias --- lib/galaxy/tool_util/parser/xml.py | 2 +- lib/galaxy/tool_util/xsd/galaxy.xsd | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tool_util/parser/xml.py b/lib/galaxy/tool_util/parser/xml.py index 88206c1ec09..88eace9a56a 100644 --- a/lib/galaxy/tool_util/parser/xml.py +++ b/lib/galaxy/tool_util/parser/xml.py @@ -598,7 +598,7 @@ def __parse_output_collection_elem(output_collection_elem, profile=None): def __parse_element_tests(parent_element, profile=None): - element_tests = OrderedDict() + element_tests = {} for idx, element in enumerate(parent_element.findall("element")): element_attrib = dict(element.attrib) identifier = element_attrib.pop('name', None) diff --git a/lib/galaxy/tool_util/xsd/galaxy.xsd b/lib/galaxy/tool_util/xsd/galaxy.xsd index 3da3646fa3e..3393572d00d 100644 --- a/lib/galaxy/tool_util/xsd/galaxy.xsd +++ b/lib/galaxy/tool_util/xsd/galaxy.xsd @@ -1507,7 +1507,7 @@ Note that this tool uses ``assign_primary_output="true"`` for ``