core/tests/test_run_tool.py should be merged into core/tests/test_tool.py

[Tobychev]

src/ctapipe/core/tests/test_run_tool.py contains a single test that easily fit inside test_tool.py both physically and logically, spreading tests of the same functionality over many files is bound to lead to confusion in the long run.

[maxnoe]

I am confused: you mention the same filename twice.

What should be merged?

[Tobychev]

@maxnoe Sorry for the confusion, I've updated the path now, I'm talking about the newly added test_run_tool.py introduced in #2566 .

[mexanick]

@Tobychev it wasn't added in #2566, it was there before... But I agree, it is redundant, its functionality shall be merged with test_tool.py

[maxnoe]

It was not added, it has been there since a very long time. test_tool has tests the tool class, test_run_tool has tests for the run_tool function.

I don't really see why it is an issue to have separate files for these tests.

[Tobychev]

Actually no, by now test_tool has many tests of just the tool, in fact it has more functions dedicated to testing the tool than test_run_tool, the latter only having the latter has only one single function right now.

So both test_run_tool and test_tool has a test_tool_exit_code function, but the latter also has test_tool_raises, test_provenance_log_help, test_tool_logging_quiet, and then there are several further test functions that also use run_tool, some even in asserts, but could maybe be argued to only be testing the tool class.

So after reviewing it, I personally find that the divide doesn't seem well respected in practice which I think indicates it wasn't really that useful.