search

cta-observatory / ctapipe

Low-level data processing pipeline software for CTAO or similar arrays of Imaging Atmospheric Cherenkov Telescopes
https://ctapipe.readthedocs.org
BSD 3-Clause "New" or "Revised" License
63 stars 266 forks source link

`core/tests/test_run_tool.py` should be merged into `core/tests/test_tool.py` #2576

Open Tobychev opened 2 weeks ago

Tobychev commented 2 weeks ago

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 commented 1 week ago

I am confused: you mention the same filename twice.

What should be merged?

Tobychev commented 1 week ago

@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 commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

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.