Exit non-zero from planemo autoupdate when a tool fails to update

HISTORY.rst

@@ -13,6 +13,8 @@ History
 * Add ``--extended_git_diff`` to ``ci_find_repos``, ``ci_find_tools`` and
   ``list_repos``, resolving each file changed in ``--changed_in_commit_range``
   to the tools and repositories that own it - fixes #1129.
+* Exit non-zero from ``planemo autoupdate`` when a tool fails to update - fixes
+  #1478.
 
 
 ---------------------

planemo/commands/cmd_autoupdate.py

@@ -101,23 +101,28 @@ def cli(ctx, paths, **kwds):  # noqa C901
         for tool_path, tool_xml in yield_tool_sources_on_paths(ctx, paths, recursive):
             if tool_path.split("/")[-1] in tools_to_skip:
                 info("Skipping tool %s" % tool_path)
+                # a skipped tool is still a target - don't let it trip assert_at_least_one
+                exit_codes.append(EXIT_CODE_OK)
                 continue
             info("Auto-updating tool %s" % tool_path)
+            failed = False
             try:
                 updated = autoupdate.autoupdate_tool(ctx, tool_path, modified_files=modified_files, **kwds)
                 if updated:
                     modified_files.update(updated)
             except Exception as e:
                 error(f"{tool_path} could not be updated - the following error was raised: {e.__str__()}")
-            if handle_tool_load_error(tool_path, tool_xml):
+                failed = True
+            if handle_tool_load_error(tool_path, tool_xml) or failed:
                 exit_codes.append(EXIT_CODE_GENERIC_FAILURE)
-                continue
             else:
                 exit_codes.append(EXIT_CODE_OK)
 
-    workflows = [
-        r for r in runnables if r.type == RunnableType.galaxy_workflow and r.path.split("/")[-1] not in tools_to_skip
-    ]
+    all_workflows = [r for r in runnables if r.type == RunnableType.galaxy_workflow]
+    workflows = [r for r in all_workflows if r.path.split("/")[-1] not in tools_to_skip]
+    # workflows, skipped ones included, are targets too - record them for assert_at_least_one below
+    exit_codes.extend([EXIT_CODE_OK] * len(all_workflows))
+
     modified_workflows = []
     for workflow in workflows:
         tools_to_update = autoupdate.get_tools_to_update(ctx, workflow, tools_to_skip)
@@ -172,7 +177,7 @@ def cli(ctx, paths, **kwds):  # noqa C901
             kwds["engine"] = "galaxy"
             return_value = test_runnables(ctx, runnables, original_paths=paths, **kwds)
             exit_codes.append(return_value)
-    return coalesce_return_codes(exit_codes, assert_at_least_one=assert_tools)
+    ctx.exit(coalesce_return_codes(exit_codes, assert_at_least_one=assert_tools))
 
 
 def handle_tool_load_error(tool_path: str, tool_xml: XmlToolSource) -> bool:

tests/test_cmd_autoupdate.py

@@ -90,6 +90,36 @@ def test_autoupdate_no_update_needed(self):
             result = self._runner.invoke(self._cli.planemo, autoupdate_command)
             assert f"No updates required or made to {xmlfile}." in result.output
 
+    def test_autoupdate_malformed_xml(self):
+        """Test autoupdate exits non-zero when a tool cannot be parsed."""
+        with self._isolate_repo("bad_invalid_tool_xml") as f:
+            tool_path = os.path.realpath(os.path.join(f, "cat.xml"))
+            result = self._check_exit_code(["autoupdate", tool_path], exit_code=1)
+            assert f"Could not update {tool_path} due to malformed xml." in result.output
+
+    def test_autoupdate_no_targets(self):
+        """Test autoupdate on a path containing nothing to update."""
+        with self._isolate() as f:
+            self._check_exit_code(["autoupdate", f], exit_code=2)
+
+    def test_autoupdate_skiplist_only_tool(self):
+        """Test autoupdate on a repo whose only tool is skiplisted."""
+        with self._isolate_repo("single_tool") as f:
+            skiplist = os.path.join(f, "skiplist.txt")
+            with open(skiplist, "w") as handle:
+                handle.write("cat.xml\n")
+            result = self._check_exit_code(["autoupdate", f, "--skiplist", skiplist], exit_code=0)
+            assert "Skipping tool" in result.output
+
+    def test_autoupdate_skiplist_only_workflow(self):
+        """Test autoupdate on a path whose only workflow is skiplisted."""
+        with self._isolate_with_test_data("wf_repos/autoupdate_tests") as f:
+            wf_file = os.path.join(f, "workflow_with_unexisting_tool.ga")
+            skiplist = os.path.join(f, "skiplist.txt")
+            with open(skiplist, "w") as handle:
+                handle.write("workflow_with_unexisting_tool.ga\n")
+            self._check_exit_code(["autoupdate", wf_file, "--skiplist", skiplist], exit_code=0)
+
     @skip_if_environ("PLANEMO_SKIP_GALAXY_TESTS")
     def test_autoupdate_multiple_workflows(self):
         """Test autoupdate command for a workflow is needed."""
@@ -179,3 +209,5 @@ def test_autoupdate_workflow_unexisting_tool(self):
             autoupdate_command = ["autoupdate", wf_file]
             result = self._runner.invoke(self._cli.planemo, autoupdate_command)
             assert "No newer tool versions were found, so the workflow was not updated." in result.output
+            # a workflow-only autoupdate found a target, so it must not report "no such target"
+            assert result.exit_code == 0