From bb7de2c0db60d6a69d84401279830fa87e8d9000 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 28 Feb 2018 14:58:37 -0500 Subject: [PATCH] Improved tool test API. Cleanup up internal representations used to bridge the tool parser and the tool testing modules to produce more sensible dictified representations when exported via the API. This is mostly replacing tuples (which get converted to JSON lists) to dicts but there are some other small cleanups. --- lib/galaxy/tools/parser/interface.py | 34 ++++++++++---------- lib/galaxy/tools/parser/xml.py | 21 +++++++++---- lib/galaxy/tools/parser/yaml.py | 10 ++++-- lib/galaxy/tools/test.py | 32 +++++++++++-------- lib/galaxy/tools/verify/interactor.py | 42 ++++++++++++++----------- test/unit/tools/test_parsing.py | 45 +++++++++++++++++---------- 6 files changed, 109 insertions(+), 75 deletions(-) diff --git a/lib/galaxy/tools/parser/interface.py b/lib/galaxy/tools/parser/interface.py index 807ebb7064d..0ca8f33187e 100644 --- a/lib/galaxy/tools/parser/interface.py +++ b/lib/galaxy/tools/parser/interface.py @@ -375,9 +375,10 @@ class TestCollectionDef(object): element_identifier = element_attrib["name"] nested_collection_elem = element.find("collection") if nested_collection_elem is not None: - elements.append((element_identifier, TestCollectionDef.from_xml(nested_collection_elem, parse_param_elem))) + element_definition = TestCollectionDef.from_xml(nested_collection_elem, parse_param_elem) else: - elements.append((element_identifier, parse_param_elem(element))) + element_definition = parse_param_elem(element) + elements.append({"element_identifier": element_identifier, "element_definition": element_definition}) return TestCollectionDef( attrib=attrib, @@ -387,18 +388,18 @@ class TestCollectionDef(object): ) def to_dict(self): - def element_to_dict(element_pair): - element_identifier, element_def = element_pair + def element_to_dict(element_dict): + element_identifier, element_def = element_dict["element_identifier"], element_dict["element_definition"] if isinstance(element_def, TestCollectionDef): element_def = element_def.to_dict() return { "element_identifier": element_identifier, - "element_def": element_def, + "element_definition": element_def, } return { "model_class": "TestCollectionDef", - "attrib": self.attrib, + "attributes": self.attrib, "collection_type": self.collection_type, "elements": map(element_to_dict, self.elements or []), "name": self.name, @@ -409,27 +410,24 @@ class TestCollectionDef(object): assert as_dict["model_class"] == "TestCollectionDef" def element_from_dict(element_dict): - if "element_def" not in element_dict: + if "element_definition" not in element_dict: raise Exception("Invalid element_dict %s" % element_dict) - element_def = element_dict["element_def"] - # TODO: stop using tuples and use dicts internally to eliminate this check - if not isinstance(element_def, dict): - pass - elif element_def.get("model_class", None) == "TestCollectionDef": + element_def = element_dict["element_definition"] + if element_def.get("model_class", None) == "TestCollectionDef": element_def = TestCollectionDef.from_dict(element_def) - return (element_dict["element_identifier"], element_def) + return {"element_identifier": element_dict["element_identifier"], "element_definition": element_def} return TestCollectionDef( - attrib=as_dict["attrib"], + attrib=as_dict["attributes"], name=as_dict["name"], - elements=map(element_from_dict, as_dict["elements"] or []), + elements=list(map(element_from_dict, as_dict["elements"] or [])), collection_type=as_dict["collection_type"], ) def collect_inputs(self): inputs = [] for element in self.elements: - value = element[1] + value = element["element_definition"] if isinstance(value, TestCollectionDef): inputs.extend(value.collect_inputs()) else: @@ -451,13 +449,13 @@ class TestCollectionOutputDef(object): def from_dict(as_dict): return TestCollectionOutputDef( name=as_dict["name"], - attrib=as_dict["attrib"], + attrib=as_dict["attributes"], element_tests=as_dict["element_tests"], ) def to_dict(self): return dict( name=self.name, - attrib=self.attrib, + attributes=self.attrib, element_tests=self.element_tests ) diff --git a/lib/galaxy/tools/parser/xml.py b/lib/galaxy/tools/parser/xml.py index 14ef66932c3..fde2b532935 100644 --- a/lib/galaxy/tools/parser/xml.py +++ b/lib/galaxy/tools/parser/xml.py @@ -416,7 +416,7 @@ def _test_elem_to_dict(test_elem, i): expect_failure=string_as_bool(test_elem.get("expect_failure", False)), maxseconds=test_elem.get("maxseconds", None), ) - _copy_to_dict_if_present(test_elem, rval, ["interactor", "num_outputs"]) + _copy_to_dict_if_present(test_elem, rval, ["num_outputs"]) return rval @@ -429,7 +429,7 @@ def __parse_output_elems(test_elem): outputs = [] for output_elem in test_elem.findall("output"): name, file, attributes = __parse_output_elem(output_elem) - outputs.append([name, file, attributes]) + outputs.append({"name": name, "value": file, "attributes": attributes}) return outputs @@ -563,7 +563,12 @@ def __parse_extra_files_elem(extra): assert extra_type == 'directory' or extra_name is not None, \ 'extra_files type (%s) requires a name attribute' % extra_type extra_value, extra_attributes = __parse_test_attributes(extra, attrib) - return extra_type, extra_value, extra_name, extra_attributes + return { + "value": extra_value, + "name": extra_name, + "type": extra_type, + "attributes": extra_attributes + } def __expand_input_elems(root_elem, prefix=""): @@ -626,8 +631,8 @@ def _copy_to_dict_if_present(elem, rval, attributes): def __parse_inputs_elems(test_elem, i): raw_inputs = [] for param_elem in test_elem.findall("param"): - name, value, attrib = __parse_param_elem(param_elem, i) - raw_inputs.append((name, value, attrib)) + raw_inputs.append(__parse_param_elem(param_elem, i)) + return raw_inputs @@ -671,7 +676,11 @@ def __parse_param_elem(param_elem, i=0): # take precedence attrib['edit_attributes'].insert(0, {'type': 'name', 'value': composite_data_name}) name = attrib.pop('name') - return (name, value, attrib) + return { + "name": name, + "value": value, + "attributes": attrib + } class StdioParser(object): diff --git a/lib/galaxy/tools/parser/yaml.py b/lib/galaxy/tools/parser/yaml.py index 71a593e3cf6..4a8d796e273 100644 --- a/lib/galaxy/tools/parser/yaml.py +++ b/lib/galaxy/tools/parser/yaml.py @@ -188,7 +188,7 @@ def _parse_test(i, test_dict): if _is_dict(inputs): new_inputs = [] for key, value in inputs.items(): - new_inputs.append((key, value, {})) + new_inputs.append({"name": key, "value": value, "attributes": {}}) test_dict["inputs"] = new_inputs outputs = test_dict["outputs"] @@ -202,7 +202,11 @@ def _parse_test(i, test_dict): else: file = value attributes = {} - new_outputs.append((key, file, attributes)) + new_outputs.append({ + "name": key, + "value": file, + "attributes": attributes + }) else: for output in outputs: name = output["name"] @@ -211,7 +215,7 @@ def _parse_test(i, test_dict): new_outputs.append((name, value, attributes)) for output in new_outputs: - attributes = output[2] + attributes = output["attributes"] defaults = { 'compare': 'diff', 'lines_diff': 0, diff --git a/lib/galaxy/tools/test.py b/lib/galaxy/tools/test.py index 56b670f1889..a26f26fea9d 100644 --- a/lib/galaxy/tools/test.py +++ b/lib/galaxy/tools/test.py @@ -40,8 +40,9 @@ def description_from_tool_object(tool, test_index, raw_test_dict): num_outputs = int(num_outputs) try: + processed_inputs = _process_raw_inputs(tool, tool.inputs, raw_test_dict["inputs"], required_files) processed_test_dict = { - "inputs": _process_raw_inputs(tool, tool.inputs, raw_test_dict["inputs"], required_files), + "inputs": processed_inputs, "outputs": raw_test_dict["outputs"], "output_collections": raw_test_dict["output_collections"], "num_outputs": num_outputs, @@ -50,13 +51,13 @@ def description_from_tool_object(tool, test_index, raw_test_dict): "stderr": raw_test_dict.get("stderr", None), "expect_exit_code": raw_test_dict.get("expect_exit_code", None), "expect_failure": raw_test_dict.get("expect_failure", False), - "md5": raw_test_dict.get("md5", None), "required_files": required_files, "tool_id": tool.id, "test_index": test_index, "error": False, } except Exception as e: + log.exception("Failed to load tool test number [%d] for %s" % (test_index, tool.id)) processed_test_dict = { "tool_id": tool.id, "test_index": test_index, @@ -80,8 +81,8 @@ def _process_raw_inputs(tool, tool_inputs, raw_inputs, required_files, parent_co if isinstance(value, galaxy.tools.parameters.grouping.Conditional): cond_context = ParamContext(name=value.name, parent_context=parent_context) case_context = ParamContext(name=value.test_param.name, parent_context=cond_context) - raw_input = case_context.extract_value(raw_inputs) - case_value = raw_input[1] if raw_input else None + raw_input_dict = case_context.extract_value(raw_inputs) + case_value = raw_input_dict["value"] if raw_input_dict else None case = _matching_case_for_value(tool, value, case_value) if case: for input_name, input_value in case.inputs.items(): @@ -120,9 +121,11 @@ def _process_raw_inputs(tool, tool_inputs, raw_inputs, required_files, parent_co repeat_index += 1 else: context = ParamContext(name=value.name, parent_context=parent_context) - raw_input = context.extract_value(raw_inputs) - if raw_input: - (name, param_value, param_extra) = raw_input + raw_input_dict = context.extract_value(raw_inputs) + if raw_input_dict: + name = raw_input_dict["name"] + param_value = raw_input_dict["value"] + param_extra = raw_input_dict["attributes"] if not value.type == "text": param_value = _split_if_str(param_value) if isinstance(value, galaxy.tools.parameters.basic.DataToolParameter): @@ -133,8 +136,11 @@ def _process_raw_inputs(tool, tool_inputs, raw_inputs, required_files, parent_co elif isinstance(value, galaxy.tools.parameters.basic.DataCollectionToolParameter): assert 'collection' in param_extra collection_def = param_extra['collection'] - for (name, value, extra) in collection_def.collect_inputs(): - require_file(name, value, extra, required_files) + for input_dict in collection_def.collect_inputs(): + name = input_dict["name"] + value = input_dict["value"] + attributes = input_dict["attributes"] + require_file(name, value, attributes, required_files) processed_value = collection_def else: processed_value = _process_simple_value(value, param_value) @@ -309,13 +315,13 @@ class ParamContext(object): def __raw_param_found(self, param_name, raw_inputs): index = None - for i, raw_input in enumerate(raw_inputs): - if raw_input[0] == param_name: + for i, raw_input_dict in enumerate(raw_inputs): + if raw_input_dict["name"] == param_name: index = i if index is not None: - raw_input = raw_inputs[index] + raw_input_dict = raw_inputs[index] del raw_inputs[index] - return raw_input + return raw_input_dict else: return None diff --git a/lib/galaxy/tools/verify/interactor.py b/lib/galaxy/tools/verify/interactor.py index 8dcac95c7fe..e60da81a26f 100644 --- a/lib/galaxy/tools/verify/interactor.py +++ b/lib/galaxy/tools/verify/interactor.py @@ -342,19 +342,20 @@ class GalaxyInteractorApi(object): def _element_identifiers(self, collection_def): element_identifiers = [] - for (element_identifier, element) in collection_def.elements: - if isinstance(element, TestCollectionDef): - subelement_identifiers = self._element_identifiers(element) + for element_dict in collection_def.elements: + element_identifier = element_dict["element_identifier"] + element_def = element_dict["element_definition"] + if isinstance(element_def, TestCollectionDef): + subelement_identifiers = self._element_identifiers(element_def) element = dict( name=element_identifier, src="new_collection", - collection_type=element.collection_type, + collection_type=element_def.collection_type, element_identifiers=subelement_identifiers ) else: - element_name = element[0] - element = self.uploads[element[1]].copy() - element["name"] = element_name + element = self.uploads[element_def["value"]].copy() + element["name"] = element_identifier element_identifiers.append(element) return element_identifiers @@ -621,14 +622,19 @@ def _verify_composite_datatype_file_content(file_name, hda_id, base_name=None, a def _verify_extra_files_content(extra_files, hda_id, dataset_fetcher, test_data_path_builder, keep_outputs_dir): files_list = [] - for extra_type, extra_value, extra_name, extra_attributes in extra_files: - if extra_type == 'file': - files_list.append((extra_name, extra_value, extra_attributes)) - elif extra_type == 'directory': - for filename in os.listdir(test_data_path_builder(extra_value)): - files_list.append((filename, os.path.join(extra_value, filename), extra_attributes)) + for extra_file_dict in extra_files: + extra_file_type = extra_file_dict["type"] + extra_file_name = extra_file_dict["name"] + extra_file_attributes = extra_file_dict["attributes"] + extra_file_value = extra_file_dict["value"] + + if extra_file_type == 'file': + files_list.append((extra_file_name, extra_file_value, extra_file_attributes)) + elif extra_file_type == 'directory': + for filename in os.listdir(test_data_path_builder(extra_file_value)): + files_list.append((filename, os.path.join(extra_file_value, filename), extra_file_attributes)) else: - raise ValueError('unknown extra_files type: %s' % extra_type) + raise ValueError('unknown extra_files type: %s' % extra_file_type) for filename, filepath, attributes in files_list: _verify_composite_datatype_file_content(filepath, hda_id, base_name=filename, attributes=attributes, dataset_fetcher=dataset_fetcher, test_data_path_builder=test_data_path_builder, keep_outputs_dir=keep_outputs_dir) @@ -758,9 +764,11 @@ def _verify_outputs(testdef, history, jobs, tool_id, data_list, data_collection_ error = AssertionError("Expected job to complete with exit code %s, found %s" % (expect_exit_code, exit_code)) register_exception(error) - for output_index, output_tuple in enumerate(testdef.outputs): + for output_index, output_dict in enumerate(testdef.outputs): # Get the correct hid - name, outfile, attributes = output_tuple + name = output_dict["name"] + outfile = output_dict["value"] + attributes = output_dict["attributes"] output_testdef = Bunch(name=name, outfile=outfile, attributes=attributes) try: output_data = data_list[name] @@ -922,7 +930,6 @@ class ToolTestDescription(object): self.stderr = processed_test_dict.get("stderr", None) self.expect_exit_code = processed_test_dict.get("expect_exit_code", None) self.expect_failure = processed_test_dict.get("expect_failure", False) - self.md5 = processed_test_dict.get("md5", None) def test_data(self): """ @@ -948,7 +955,6 @@ class ToolTestDescription(object): "stderr": self.stderr, "expect_exit_code": self.expect_exit_code, "expect_failure": self.expect_failure, - "md5": self.md5, "name": self.name, "test_index": self.test_index, "tool_id": self.tool_id, diff --git a/test/unit/tools/test_parsing.py b/test/unit/tools/test_parsing.py index 6086ac555f5..abc966e3b5a 100644 --- a/test/unit/tools/test_parsing.py +++ b/test/unit/tools/test_parsing.py @@ -201,17 +201,17 @@ class XmlLoaderTestCase(BaseLoaderTestCase): assert len(tests) == 2 test_dict = tests[0] inputs = test_dict["inputs"] - assert len(inputs) == 1 + assert len(inputs) == 1, test_dict input1 = inputs[0] - assert input1[0] == "foo" - assert input1[1] == "5" + assert input1["name"] == "foo", input1 + assert input1["value"] == "5" outputs = test_dict["outputs"] assert len(outputs) == 1 output1 = outputs[0] - assert output1[0] == 'out1' - assert output1[1] == 'moo.txt' - attributes1 = output1[2] + assert output1["name"] == 'out1' + assert output1["value"] == 'moo.txt' + attributes1 = output1["attributes"] assert attributes1["compare"] == "diff" assert attributes1["lines_diff"] == 0 @@ -219,9 +219,9 @@ class XmlLoaderTestCase(BaseLoaderTestCase): outputs = test2["outputs"] assert len(outputs) == 1 output2 = outputs[0] - assert output2[0] == 'out1' - assert output2[1] is None - attributes1 = output2[2] + assert output2["name"] == 'out1' + assert output2["value"] is None + attributes1 = output2["attributes"] assert attributes1["compare"] == "sim_size" assert attributes1["lines_diff"] == 4 @@ -350,15 +350,15 @@ class YamlLoaderTestCase(BaseLoaderTestCase): inputs = test_dict["inputs"] assert len(inputs) == 1 input1 = inputs[0] - assert input1[0] == "foo" - assert input1[1] == 5 + assert input1["name"] == "foo" + assert input1["value"] == 5 outputs = test_dict["outputs"] assert len(outputs) == 1 output1 = outputs[0] - assert output1[0] == 'out1' - assert output1[1] == 'moo.txt' - attributes1 = output1[2] + assert output1["name"] == 'out1' + assert output1["value"] == 'moo.txt' + attributes1 = output1["attributes"] assert attributes1["compare"] == "diff" assert attributes1["lines_diff"] == 0 @@ -366,9 +366,9 @@ class YamlLoaderTestCase(BaseLoaderTestCase): outputs = test2["outputs"] assert len(outputs) == 1 output2 = outputs[0] - assert output2[0] == 'out1' - assert output2[1] is None - attributes1 = output2[2] + assert output2["name"] == 'out1' + assert output2["value"] is None + attributes1 = output2["attributes"] assert attributes1["compare"] == "sim_size" assert attributes1["lines_diff"] == 4 @@ -448,3 +448,14 @@ class SpecialToolLoaderTestCase(BaseLoaderTestCase): action = self._tool_source.parse_action_module() assert action[0] == "galaxy.tools.actions.history_imp_exp" assert action[1] == "ExportHistoryToolAction" + + +class CollectionTestCase(BaseLoaderTestCase): + source_file_name = os.path.join(os.getcwd(), "test/functional/tools/collection_two_paired.xml") + source_contents = None + + def test_tests(self): + tests_dict = self._tool_source.parse_tests_to_dict() + tests = tests_dict["tests"] + assert len(tests) == 2 + assert len(tests[0]["inputs"]) == 3, tests[0]