From a8854de0aae77a9c9ed9ccc188473a3cb8c9bce5 Mon Sep 17 00:00:00 2001
From: Noah Biederbeck <noah.biederbeck@tu-dortmund.de>
Date: Wed, 21 Dec 2022 14:35:15 +0100
Subject: [PATCH 1/3] Change default behaviour of run_tool:

1. raises=True
2. if cwd is None (default), then a temporary directory is created,
   instead of using the directory from where pytest is run.
---
 ctapipe/core/tool.py | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/ctapipe/core/tool.py b/ctapipe/core/tool.py
index 46259f13d47..58d5e862409 100644
--- a/ctapipe/core/tool.py
+++ b/ctapipe/core/tool.py
@@ -7,6 +7,7 @@
 import textwrap
 from abc import abstractmethod
 from inspect import cleandoc
+from tempfile import mkdtemp
 from typing import Union
 
 import yaml
@@ -333,6 +334,9 @@ def run(self, argv=None, raises=False):
         argv: list(str)
             command-line arguments, or None to get them
             from sys.argv automatically
+
+        raises : bool
+            Whether to raise Exceptions (to test them) or not.
         """
 
         # return codes are taken from:
@@ -539,9 +543,20 @@ def commented(text, indent_level=2, width=70):
     return "\n".join(lines)
 
 
-def run_tool(tool: Tool, argv=None, cwd=None, raises=False):
+def run_tool(tool: Tool, argv=None, cwd=None, raises=True):
     """
-    Utility run a certain tool in a python session without exitinig
+    Utility run a certain tool in a python session without exiting.
+
+    Parameters
+    ----------
+    argv : List[str]
+        List of command line arguments for the tool.
+    cwd : str or pathlib.Path
+        Path to a temporary working directory. If none, a new (random)
+        temporary directeory gets created.
+    raises : bool
+        If true, raises Exceptions from running tools, to test them.
+        If false, tools can return a non-zero exit code.
 
     Returns
     -------
@@ -549,7 +564,7 @@ def run_tool(tool: Tool, argv=None, cwd=None, raises=False):
         The return code of the tool, 0 indicates success, everything else an error
     """
     current_cwd = pathlib.Path().absolute()
-    cwd = pathlib.Path(cwd) if cwd is not None else current_cwd
+    cwd = pathlib.Path(cwd) if cwd is not None else mkdtemp()
     try:
         # switch to cwd for running and back after
         os.chdir(cwd)

From df324eaf61e4a14e094daa057322861b5b880472 Mon Sep 17 00:00:00 2001
From: Noah Biederbeck <noah.biederbeck@tu-dortmund.de>
Date: Wed, 21 Dec 2022 14:37:12 +0100
Subject: [PATCH 2/3] Fix run_tool tests.

---
 ctapipe/core/tests/test_traits.py   | 6 +++---
 ctapipe/tools/tests/test_process.py | 1 +
 ctapipe/tools/tests/test_tools.py   | 1 +
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/ctapipe/core/tests/test_traits.py b/ctapipe/core/tests/test_traits.py
index ea6aad33079..45bf18372ca 100644
--- a/ctapipe/core/tests/test_traits.py
+++ b/ctapipe/core/tests/test_traits.py
@@ -534,7 +534,7 @@ class SomeComponent(TelescopeComponent):
     assert config["SomeComponent"]["tel_param1"] == [("type", "*", 6.0)]
 
 
-def test_telescope_parameter_from_cli(mock_subarray):
+def test_telescope_parameter_from_cli(tmp_path, mock_subarray):
     """
     Test we can pass single default for telescope components via cli
     see #1559
@@ -569,7 +569,7 @@ def setup(self):
             tool,
             [
                 "--SomeComponent.path",
-                "test.h5",
+                f"{tmp_path}/test.h5",
                 "--SomeComponent.val",
                 "2.0",
                 "--SomeComponent.flag",
@@ -577,7 +577,7 @@ def setup(self):
             ],
         )
         assert result == 0
-        assert tool.comp.path == [("type", "*", pathlib.Path("test.h5").absolute())]
+        assert tool.comp.path == [("type", "*", tmp_path / "test.h5")]
         assert tool.comp.val == [("type", "*", 2.0)]
         assert tool.comp.flag == [("type", "*", False)]
 
diff --git a/ctapipe/tools/tests/test_process.py b/ctapipe/tools/tests/test_process.py
index 301e8ac15e2..8f976403f41 100644
--- a/ctapipe/tools/tests/test_process.py
+++ b/ctapipe/tools/tests/test_process.py
@@ -136,6 +136,7 @@ def test_stage_1_dl1(tmp_path, dl1_image_file, dl1_parameters_file):
             "--overwrite",
         ],
         cwd=tmp_path,
+        raises=False,
     )
     assert ret == 1
 
diff --git a/ctapipe/tools/tests/test_tools.py b/ctapipe/tools/tests/test_tools.py
index 834e9f53ee2..f42f27bd395 100644
--- a/ctapipe/tools/tests/test_tools.py
+++ b/ctapipe/tools/tests/test_tools.py
@@ -99,6 +99,7 @@ def test_display_dl1(tmp_path, dl1_image_file, dl1_parameters_file):
     ret = run_tool(
         DisplayDL1Calib(),
         argv=[f"--input={dl1_parameters_file}", "--max-events=1", "--telescope=11"],
+        raises=False,
     )
     assert ret == 1
     assert run_tool(DisplayDL1Calib(), ["--help-all"]) == 0

From cbe2bb17858e1125ef37e57838bd63dc4e17daf2 Mon Sep 17 00:00:00 2001
From: Noah Biederbeck <noah.biederbeck@tu-dortmund.de>
Date: Wed, 21 Dec 2022 15:30:55 +0100
Subject: [PATCH 3/3] Add changelog entry

---
 docs/changes/2175.api.rst | 12 ++++++++++++
 1 file changed, 12 insertions(+)
 create mode 100644 docs/changes/2175.api.rst

diff --git a/docs/changes/2175.api.rst b/docs/changes/2175.api.rst
new file mode 100644
index 00000000000..08ed7e89fe8
--- /dev/null
+++ b/docs/changes/2175.api.rst
@@ -0,0 +1,12 @@
+Change default behaviour of `run_rool`:
+
+1. The default value of `raises` is now `True`. That means, when using
+   `run_tool`, the Exceptions raised by a Tool will be re-raised. The raised
+   exceptions can be tested for their type and content.
+   If the Tool must fail and only the non-zero error case is important to test,
+   set `raises=False` (as it was before).
+
+2. If the `cwd` parameter is `None` (as per default), now a temporary directory
+   is used instead of the directory, where `run_tool` is called (typically via
+   pytest). This way, log-files and other output files don't clutter your
+   working space.