Refactor tests

[sfmig]

A PR for refactoring the cellfinder workflow tests, following the feedback collected on issue #26.

Main features

They address the points from issue #26:

• define unit tests
• separate integration and unit tests
• make workflows and tests more modular:
  • added a utils.py module
  • separate general unit tests (e.g., for parser) and cellfinder-specific unit tests (with modules / directories)
• defined fixtures in the files with the test definitions
  • I kept a conftest.py for the fixtures that are shared between unit and integration tests. I also use an additional conftest.py for the fixtures shared between utils unit tests and cellfinder-specific unit tests.
  • replaced the make_config helper functions by config files that are part of the test data

Other additions

• added default config files (under brainglobe_workflows/configs)
• renamed benchmarks directory to brainglobe_benchmarks
• removed cellfinder subdirectory under brainglobe_workflows
• added an entrypoint for the workflow called cellfinder-workflow
• benchmark tests marked as skip for now (to focus on the workflow ones)

Comments / questions

• I think I went down a parametrisations 🐰 🕳️ , maybe some are redundant? Any feedback on this is welcome!
• The tests are a bit slow to run, mainly because often I download the data from GIN. Should I look into caching this? Right now I don't because I download the data to a pytest temp dir, which is different for each test run.
• I wanted to define fixtures per test module and used multiple conftest.py files to do so. But I'm not sure if that makes any difference compared to the case of having all of the fixtures in one conftest.py file (I think pytest just looks for conftest.py files and loads them all at once). Is there a better way to define fixtures per test module?
• For a few commits I replaced the use of arparse by typer, and the code look cleaner and more readable, but at the cost of adding an extra dependency. Chatting to Adam he mentioned in brainglobe we try to keep dependencies lists short, so I reverted back to argparse but let me know if you have any thoughts about it.

[codecov[bot]]

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (afba965) 80.85% compared to head (466667c) 81.31%. Report is 5 commits behind head on main.

Files	Patch %	Lines
...tegration/brainglobe_benchmarks/test_cellfinder.py	39.13%	14 Missing :warning:
brainglobe_benchmarks/cellfinder.py	0.00%	4 Missing :warning:
brainglobe_workflows/cellfinder.py	97.34%	3 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #27 +/- ## ========================================== + Coverage 80.85% 81.31% +0.45% ========================================== Files 28 32 +4 Lines 1494 1584 +90 ========================================== + Hits 1208 1288 +80 - Misses 286 296 +10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[alessandrofelder]

These two questions relate to our live discussion of where the workflow data should live, and to a further thought of mine that feels that a lot of the test code is still a bit complicated.

• I think I went down a parametrisations 🐰 🕳️ , maybe some are redundant? Any feedback on this is welcome!

I think the parametrisations you've done are fine, but I'd say the if statement distinguishing two tests is a minor code smell. Maybe split the default case off into a separate test that doesn't require parametrisation?

• The tests are a bit slow to run, mainly because often I download the data from GIN. Should I look into caching this? Right now I don't because I download the data to a pytest temp dir, which is different for each test run.

Yes, I think caching is worth it long-term to improve the development experience. If we move to using paths relative to $HOME as discussed, I would mock Path.home() as done in brainrender-napari instead of using tmp_path which would simplify both the test code and the caching..

The test code would then be something like:

def test_main(
):
    """Test main function for setting up and running cellfinder workflow (default case)
    """
    # mocking of $HOME in conftest.py takes care that test data and user data are separate
    cfg = main()

    # check output files exist
    assert (cfg.detected_cells_path).is_file()

@pytest.mark.parametrize(
    "input_config",
    [
        "input_config_fetch_GIN",
        "input_config_fetch_local",
    ],
)
def test_main_non_default(
):
    """Test main function for setting up and running cellfinder workflow
    Parameters
    ----------
    input_config : Optional[str]
        Path to input config json file
    """
    # mocking of $HOME in conftest.py takes care that test data and user data are separate
    cfg = main(str(Path.home()/".brainglobe/cellfinder/workflows"/input_config))

    # check output files exist
    assert (cfg.detected_cells_path).is_file()

This can also be part of a separate PR