From d12f21d62e5e370ad5d8b29cb674e0e26b2e9578 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 19 Dec 2019 17:53:53 +0100 Subject: [PATCH 1/3] Add test for unset optional parameters --- test/unit/tools/test_wrappers.py | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/test/unit/tools/test_wrappers.py b/test/unit/tools/test_wrappers.py index 9bb9bb53a18..b1567be682c 100644 --- a/test/unit/tools/test_wrappers.py +++ b/test/unit/tools/test_wrappers.py @@ -108,15 +108,19 @@ def test_raw_object_wrapper(): assert not false_wrapper -def valuewrapper(tool, value, paramtype): +def valuewrapper(tool, value, paramtype, optional=False): if paramtype == "integer": - parameter = IntegerToolParameter(tool, XML('')) + optional = 'optional="true"' if optional else 'value="10"' + parameter = IntegerToolParameter(tool, XML('' % optional)) elif paramtype == "text": - parameter = TextToolParameter(tool, XML('')) + optional = 'optional="true"' if optional else 'value="foo"' + parameter = TextToolParameter(tool, XML('' % optional)) elif paramtype == "float": - parameter = FloatToolParameter(tool, XML('')) + optional = 'optional="true"' if optional else 'value="10.0"' + parameter = FloatToolParameter(tool, XML('' % optional)) elif paramtype == "boolean": - parameter = BooleanToolParameter(tool, XML('')) + optional = 'optional="true"' if optional else 'value=""' + parameter = BooleanToolParameter(tool, XML('' % optional)) return InputValueWrapper(parameter, value) @@ -143,19 +147,23 @@ def test_input_value_wrapper_comparison(tool): @with_mock_tool def test_input_value_wrapper_comparison_optional(tool): - parameter = IntegerToolParameter(tool, XML('')) - wrapper = InputValueWrapper(parameter, None) + wrapper = valuewrapper(tool, None, 'integer', optional=True) assert not wrapper with pytest.raises(ValueError): int(wrapper) assert str(wrapper) == "" assert wrapper == "" # for backward-compatibility - parameter = IntegerToolParameter(tool, XML('')) - wrapper = InputValueWrapper(parameter, 0) + wrapper = valuewrapper(tool, 0, 'integer', optional=True) assert wrapper == 0 assert int(wrapper) == 0 assert str(wrapper) assert wrapper != "" # for backward-compatibility, the correct way to check if an optional integer param is not empty is to use str(wrapper) + wrapper = valuewrapper(tool, None, 'integer', optional=True) + assert wrapper != 1 + assert str(wrapper) == "" + wrapper = valuewrapper(tool, None, "boolean") + assert bool(wrapper) is False, wrapper + assert str(wrapper) == 'falsevalue' @with_mock_tool From 18fb12b6f0b85049802f67aa1f2a67206531a796 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 19 Dec 2019 17:52:01 +0100 Subject: [PATCH 2/3] Fix optional integer/float comparisons Neither of them can be cast to int/float respectively. Fixes test 3 in bowtie2, which errors with: ``` 2019-12-19 17:11:34,022 ERROR [galaxy.jobs.runners] (12) Failure preparing job Traceback (most recent call last): File "/Users/mvandenb/src/galaxy/lib/galaxy/util/__init__.py", line 1039, in unicodify value = text_type(value) File "/Users/mvandenb/src/galaxy/.venv/lib/python3.6/site-packages/Cheetah/Template.py", line 1053, in __unicode__ return getattr(self, mainMethName)() File "cheetah_DynamicallyCompiledCheetahTemplate_1576771894_0091798_17143.py", line 680, in respond File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/wrappers.py", line 94, in __ne__ return not self == other File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/wrappers.py", line 91, in __eq__ return self._get_cast_value(other) == other File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/wrappers.py", line 88, in _get_cast_value return cast.get(self.input.type, str)(self) File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/wrappers.py", line 117, in __int__ return int(float(self)) File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/wrappers.py", line 120, in __float__ return float(str(self)) ValueError: could not convert string to float: ``` --- lib/galaxy/tools/wrappers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/tools/wrappers.py b/lib/galaxy/tools/wrappers.py index a7425d50472..89776480080 100644 --- a/lib/galaxy/tools/wrappers.py +++ b/lib/galaxy/tools/wrappers.py @@ -83,7 +83,7 @@ class InputValueWrapper(ToolParameterValueWrapper): if self.input.type == 'boolean' and isinstance(other, string_types): return str(self) # For backward compatibility, allow `$wrapper != ""` for optional non-text param - if self.input.optional and self.value is None and isinstance(other, string_types): + if self.input.optional and self.value is None and self.input.type != 'boolean': return str(self) cast = { 'text': str, From 9d1cccd14d409f1464abe345ce8601c08c6a092a Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 20 Dec 2019 12:17:46 +0100 Subject: [PATCH 3/3] Cast to None for optional params if not comparing strings Proposed by @nsroanzo in https://github.com/galaxyproject/galaxy/pull/9149#discussion_r360225247 --- lib/galaxy/tools/wrappers.py | 7 +++++-- test/unit/tools/test_wrappers.py | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tools/wrappers.py b/lib/galaxy/tools/wrappers.py index 89776480080..c49702fe307 100644 --- a/lib/galaxy/tools/wrappers.py +++ b/lib/galaxy/tools/wrappers.py @@ -83,8 +83,11 @@ class InputValueWrapper(ToolParameterValueWrapper): if self.input.type == 'boolean' and isinstance(other, string_types): return str(self) # For backward compatibility, allow `$wrapper != ""` for optional non-text param - if self.input.optional and self.value is None and self.input.type != 'boolean': - return str(self) + if self.input.optional and self.value is None: + if isinstance(other, string_types): + return str(self) + else: + return None cast = { 'text': str, 'integer': int, diff --git a/test/unit/tools/test_wrappers.py b/test/unit/tools/test_wrappers.py index b1567be682c..5acea546dbd 100644 --- a/test/unit/tools/test_wrappers.py +++ b/test/unit/tools/test_wrappers.py @@ -161,6 +161,7 @@ def test_input_value_wrapper_comparison_optional(tool): wrapper = valuewrapper(tool, None, 'integer', optional=True) assert wrapper != 1 assert str(wrapper) == "" + assert wrapper == None # noqa: E711 wrapper = valuewrapper(tool, None, "boolean") assert bool(wrapper) is False, wrapper assert str(wrapper) == 'falsevalue'