Repository: cta-observatory/ctapipe
Pull request #2191 (2023-01-06 13:45:45)

Raise error if run_tool encounters non-zero exit code and raises is True
A tool could return 2 for e.g. an unknown command line argument and not raise in `raises=True` mode, resulting in the failure to go unnoticed.


Patch:
diff --git a/ctapipe/core/tool.py b/ctapipe/core/tool.py
index 58d5e862..78f41ed4 100644
--- a/ctapipe/core/tool.py
+++ b/ctapipe/core/tool.py
@@ -7,6 +7,7 @@ import re
 import textwrap
 from abc import abstractmethod
 from inspect import cleandoc
+from subprocess import CalledProcessError
 from tempfile import mkdtemp
 from typing import Union
 
@@ -368,6 +369,8 @@ class Tool(Application):
             self.log.error("%s", err)
             self.log.error("Use --help for more info")
             exit_status = 2  # wrong cmd line parameter
+            if raises:
+                raise
         except KeyboardInterrupt:
             self.log.warning("WAS INTERRUPTED BY CTRL-C")
             Provenance().finish_activity(activity_name=self.name, status="interrupted")
@@ -377,7 +380,7 @@ class Tool(Application):
             Provenance().finish_activity(activity_name=self.name, status="error")
             exit_status = 1  # any other error
             if raises:
-                raise err
+                raise
         finally:
             if not {"-h", "--help", "--help-all"}.intersection(self.argv):
                 self.write_provenance()
@@ -565,11 +568,14 @@ def run_tool(tool: Tool, argv=None, cwd=None, raises=True):
     """
     current_cwd = pathlib.Path().absolute()
     cwd = pathlib.Path(cwd) if cwd is not None else mkdtemp()
+    argv = argv or []
     try:
         # switch to cwd for running and back after
         os.chdir(cwd)
-        tool.run(argv or [], raises=raises)
+        tool.run(argv, raises=raises)
     except SystemExit as e:
+        if raises and e.code != 0:
+            raise CalledProcessError(e.code, [tool.name] + argv)
         return e.code
     finally:
         os.chdir(current_cwd)


Test patch:
diff --git a/ctapipe/core/tests/test_run_tool.py b/ctapipe/core/tests/test_run_tool.py
new file mode 100644
index 00000000..4f13676d
--- /dev/null
+++ b/ctapipe/core/tests/test_run_tool.py
@@ -0,0 +1,19 @@
+from subprocess import CalledProcessError
+
+import pytest
+
+from ctapipe.core.tool import Tool, run_tool
+
+
+def test_run_tool_raises_exit_code():
+    class ErrorTool(Tool):
+        def setup(self):
+            pass
+
+        def start(self):
+            pass
+
+    ret = run_tool(ErrorTool(), ["--non-existing-alias"], raises=False)
+    assert ret == 2
+    with pytest.raises(CalledProcessError):
+        run_tool(ErrorTool(), ["--non-existing-alias"], raises=True)
diff --git a/ctapipe/core/tests/test_tool.py b/ctapipe/core/tests/test_tool.py
index 26df23c0..247348c1 100644
--- a/ctapipe/core/tests/test_tool.py
+++ b/ctapipe/core/tests/test_tool.py
@@ -9,7 +9,11 @@ from traitlets import Dict, Float, Int, TraitError
 from traitlets.config import Config
 
 from .. import Component, Tool
-from ..tool import export_tool_config_to_commented_yaml, run_tool
+from ..tool import (
+    ToolConfigurationError,
+    export_tool_config_to_commented_yaml,
+    run_tool,
+)
 
 
 def test_tool_simple():
@@ -193,8 +197,8 @@ def test_tool_exit_code():
 
     assert exc.value.code == 1
 
-    assert run_tool(tool, ["--help"]) == 0
-    assert run_tool(tool, ["--non-existent-option"]) == 2
+    assert run_tool(tool, ["--help"], raises=False) == 0
+    assert run_tool(tool, ["--non-existent-option"], raises=False) == 2
 
 
 def test_tool_command_line_precedence():
@@ -334,14 +338,19 @@ def test_invalid_traits(tmp_path, caplog):
         param = Float(5.0, help="parameter").tag(config=True)
 
     # 2 means trait error
-    assert run_tool(MyTool(), ["--MyTool.foo=5"]) == 2
+    assert run_tool(MyTool(), ["--MyTool.foo=5"], raises=False) == 2
+
+    with pytest.raises(ToolConfigurationError):
+        run_tool(MyTool(), ["--MyTool.foo=5"], raises=True)
 
     # test that it also works for config files
     config = tmp_path / "config.json"
     with config.open("w") as f:
         json.dump({"MyTool": {"foo": 5}}, f)
 
-    assert run_tool(MyTool(), [f"--config={config}"]) == 2
+    assert run_tool(MyTool(), [f"--config={config}"], raises=False) == 2
+    with pytest.raises(ToolConfigurationError):
+        assert run_tool(MyTool(), [f"--config={config}"], raises=True)
 
 
 def test_tool_raises():
diff --git a/ctapipe/tools/tests/test_process.py b/ctapipe/tools/tests/test_process.py
index 8f976403..d836a509 100644
--- a/ctapipe/tools/tests/test_process.py
+++ b/ctapipe/tools/tests/test_process.py
@@ -4,6 +4,8 @@
 Test ctapipe-process on a few different use cases
 """
 
+from subprocess import CalledProcessError
+
 import numpy as np
 import pandas as pd
 import pytest
@@ -152,7 +154,7 @@ def test_stage1_datalevels(tmp_path):
     config = resource_file("stage1_config.json")
     tool = ProcessorTool()
 
-    assert (
+    with pytest.raises(CalledProcessError):
         run_tool(
             tool,
             argv=[
@@ -164,8 +166,7 @@ def test_stage1_datalevels(tmp_path):
             ],
             cwd=tmp_path,
         )
-        == 1
-    )
+
     # make sure the dummy event source was really used
     assert isinstance(tool.event_source, DummyEventSource)