From 93b417643eec45dd157e6acf1308e2e2e82ca242 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Mon, 29 Nov 2021 11:17:40 +0100
Subject: [PATCH] general linter: full test coverage

- more specific error message for whitespace in pre/suffix in tool
version and name
- do not pront valid tool id message if whitespaces are found
---
 lib/galaxy/tool_util/linters/general.py  | 11 ++--
 test/unit/tool_util/test_tool_linters.py | 71 ++++++++++++++++++------
 2 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/lib/galaxy/tool_util/linters/general.py b/lib/galaxy/tool_util/linters/general.py
index 62184ee2a79..878f7e9d736 100644
--- a/lib/galaxy/tool_util/linters/general.py
+++ b/lib/galaxy/tool_util/linters/general.py
@@ -19,6 +19,7 @@ PROFILE_INFO_SPECIFIED_MSG = "Tool specifies profile version [%s]."
 PROFILE_INVALID_MSG = "Tool specifies an invalid profile version [%s]."
 
 WARN_WHITESPACE_MSG = "%s contains whitespace, this may cause errors: [%s]."
+WARN_WHITESPACE_PRESUFFIX = "%s is pre/suffixed by whitespace, this may cause errors: [%s]."
 WARN_ID_WHITESPACE_MSG = (
     "Tool ID contains whitespace - this is discouraged: [%s].")
 
@@ -40,7 +41,7 @@ def lint_general(tool_source, lint_ctx):
     elif isinstance(parsed_version, packaging.version.LegacyVersion):
         lint_ctx.warn(WARN_VERSION_MSG % version, line=tool_line)
     elif version != version.strip():
-        lint_ctx.warn(WARN_WHITESPACE_MSG % ('Tool version', version), line=tool_line)
+        lint_ctx.warn(WARN_WHITESPACE_PRESUFFIX % ('Tool version', version), line=tool_line)
     else:
         lint_ctx.valid(VALID_VERSION_MSG % version, line=tool_line)
 
@@ -48,22 +49,22 @@ def lint_general(tool_source, lint_ctx):
     if not name:
         lint_ctx.error(ERROR_NAME_MSG, line=tool_line)
     elif name != name.strip():
-        lint_ctx.warn(WARN_WHITESPACE_MSG % ('Tool name', name), line=tool_line)
+        lint_ctx.warn(WARN_WHITESPACE_PRESUFFIX % ('Tool name', name), line=tool_line)
     else:
         lint_ctx.valid(VALID_NAME_MSG % name, line=tool_line)
 
     tool_id = tool_source.parse_id()
     if not tool_id:
         lint_ctx.error(ERROR_ID_MSG, line=tool_line)
+    elif re.search(r"\s", tool_id):
+        lint_ctx.warn(WARN_ID_WHITESPACE_MSG % tool_id, line=tool_line)
     else:
         lint_ctx.valid(VALID_ID_MSG % tool_id, line=tool_line)
-        if re.search(r"\s", tool_id):
-            lint_ctx.warn(WARN_ID_WHITESPACE_MSG % tool_id, line=tool_line)
 
     profile = tool_source.parse_profile()
     profile_valid = PROFILE_PATTERN.match(profile) is not None
     if not profile_valid:
-        lint_ctx.error(PROFILE_INVALID_MSG, line=tool_line)
+        lint_ctx.error(PROFILE_INVALID_MSG % profile, line=tool_line)
     elif profile == "16.01":
         lint_ctx.valid(PROFILE_INFO_DEFAULT_MSG, line=tool_line)
     else:
diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py
index ca1f450e6f1..e0a51072fd0 100644
--- a/test/unit/tool_util/test_tool_linters.py
+++ b/test/unit/tool_util/test_tool_linters.py
@@ -13,18 +13,21 @@ from galaxy.tool_util.parser.xml import XmlToolSource
 from galaxy.util import etree
 
 # tests tool xml for general linter
-WHITESPACE_IN_VERSIONS_AND_NAMES = """
-<tool name=" BWA Mapper " id="bwa tool" version=" 1.0.1 " display_interface="true" require_login="true" hidden="true">
-    <description>The BWA Mapper</description>
+GENERAL_MISSING_TOOL_ID_NAME_VERSION = """
+<tool profile="2109">
+</tool>
+"""
+
+GENERAL_WHITESPACE_IN_VERSIONS_AND_NAMES = """
+<tool name=" BWA Mapper " id="bwa tool" version=" 1.0.1 " is_multi_byte="true" display_interface="true" require_login="true" hidden="true">
     <requirements>
         <requirement type="package" version=" 1.2.5 "> bwa </requirement>
     </requirements>
 </tool>
 """
 
-REQUIREMENT_WO_VERSION = """
-<tool name="BWA Mapper" id="bwa_tool" version="1.0.1" display_interface="true" require_login="true" hidden="true">
-    <description>The BWA Mapper</description>
+GENERAL_REQUIREMENT_WO_VERSION = """
+<tool name="BWA Mapper" id="bwa_tool" version="1.0.1blah" is_multi_byte="true" display_interface="true" require_login="true" hidden="true" profile="20.09">
     <requirements>
         <requirement type="package">bwa</requirement>
         <requirement type="package" version="1.2.5"></requirement>
@@ -32,7 +35,12 @@ REQUIREMENT_WO_VERSION = """
 </tool>
 """
 
-# test tool xml for inputs linter  
+GENERAL_VALID = """
+<tool name="valid name" id="valid_id" version="1.0+galaxy1" profile="21.09">
+</tool>
+"""
+
+# test tool xml for inputs linter
 NO_INPUTS_SECTION_XML = """
 <tool>
 </tool>
@@ -271,20 +279,43 @@ TESTS_EXPECT_FAILURE_OUTPUT = """
 
 TESTS = [
     (
-        WHITESPACE_IN_VERSIONS_AND_NAMES, general.lint_general,
+        GENERAL_MISSING_TOOL_ID_NAME_VERSION, general.lint_general,
         lambda x:
-            "Tool version contains whitespace, this may cause errors: [ 1.0.1 ]." in x.warn_messages
-            and "Tool name contains whitespace, this may cause errors: [ BWA Mapper ]." in x.warn_messages
-            and "Requirement version contains whitespace, this may cause errors: [ 1.2.5 ]." in x.warn_messages
-            and "Tool ID contains whitespace - this is discouraged: [bwa tool]." in x.warn_messages
-            and len(x.warn_messages) == 4 and len(x.error_messages) == 0
+            'Tool version is missing or empty.' in x.error_messages
+            and 'Tool name is missing or empty.' in x.error_messages
+            and 'Tool does not define an id attribute.' in x.error_messages
+            and 'Tool specifies an invalid profile version [2109].' in x.error_messages
+            and len(x.info_messages) == 0 and len(x.valid_messages) == 0 and len(x.warn_messages) == 0 and len(x.error_messages) == 4
     ),
     (
-        REQUIREMENT_WO_VERSION, general.lint_general,
+        GENERAL_WHITESPACE_IN_VERSIONS_AND_NAMES, general.lint_general,
         lambda x:
-            "Requirement bwa defines no version" in x.warn_messages
+            "Tool version is pre/suffixed by whitespace, this may cause errors: [ 1.0.1 ]." in x.warn_messages
+            and "Tool name is pre/suffixed by whitespace, this may cause errors: [ BWA Mapper ]." in x.warn_messages
+            and "Requirement version contains whitespace, this may cause errors: [ 1.2.5 ]." in x.warn_messages
+            and "Tool ID contains whitespace - this is discouraged: [bwa tool]." in x.warn_messages
+            and "Tool targets 16.01 Galaxy profile." in x.valid_messages
+            and len(x.info_messages) == 0 and len(x.valid_messages) == 1 and len(x.warn_messages) == 4 and len(x.error_messages) == 0
+    ),
+    (
+        GENERAL_REQUIREMENT_WO_VERSION, general.lint_general,
+        lambda x:
+            'Tool version [1.0.1blah] is not compliant with PEP 440.' in x.warn_messages
+            and "Requirement bwa defines no version" in x.warn_messages
             and "Requirement without name found" in x.error_messages
-            and len(x.warn_messages) == 1 and len(x.error_messages) == 1
+            and "Tool specifies profile version [20.09]." in x.valid_messages
+            and "Tool defines an id [bwa_tool]." in x.valid_messages
+            and "Tool defines a name [BWA Mapper]." in x.valid_messages
+            and len(x.info_messages) == 0 and len(x.valid_messages) == 3 and len(x.warn_messages) == 2 and len(x.error_messages) == 1
+    ),
+    (
+        GENERAL_VALID, general.lint_general,
+        lambda x:
+            'Tool defines a version [1.0+galaxy1].' in x.valid_messages
+            and "Tool specifies profile version [21.09]." in x.valid_messages
+            and "Tool defines an id [valid_id]." in x.valid_messages
+            and "Tool defines a name [valid name]." in x.valid_messages
+            and len(x.info_messages) == 0 and len(x.valid_messages) == 4 and len(x.warn_messages) == 0 and len(x.error_messages) == 0
     ),
     (
         NO_INPUTS_SECTION_XML, inputs.lint_inputs,
@@ -394,8 +425,10 @@ TESTS = [
 ]
 
 TEST_IDS = [
-    'hazardous whitespace',
-    'requirement without version',
+    'general: missing tool id, name, version; invalid profile',
+    'general: whitespace in version, id, name',
+    'general: requirement without version',
+    'general: valid name, id, profile',
     'lint no sections',
     'input with redundant name',
     'lint no when',
@@ -425,6 +458,8 @@ def test_tool_xml(tool_xml, lint_func, assert_func):
         lint_target = XmlToolSource(lint_target)
     lint_ctx.lint(name="test_lint", lint_func=lint_func, lint_target=lint_target)
     assert assert_func(lint_ctx), (
+        f"Valid: {lint_ctx.valid_messages}\n"
+        f"Info: {lint_ctx.info_messages}\n"
         f"Warnings: {lint_ctx.warn_messages}\n"
         f"Errors: {lint_ctx.error_messages}"
     )