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]

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 ⚠️
brainglobe_benchmarks/cellfinder.py	0.00%	4 Missing ⚠️
brainglobe_workflows/cellfinder.py	97.34%	3 Missing ⚠️

Additional details and impacted files

@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alessandrofelder]

Looking really good - a lot of my comments are just suggestions/requests for clarifying docstrings. Some thoughts to your questions below:

• 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?

Even if pytest loads them all at once, having the conftest.py per subfolder still expresses the developer's intent as to where they should be used? In addition, I'd say keep fixtures that are used in just one file in that file (until they are needed in several files of the same 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.

I'd agree with Adam that minimising the number of dependencies is worth it here. As an additional benefit, I'd say that argparse is the de facto default for this (as part of the Python standard library) so future contributors are less likely to need to look up what typer is.

[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

[alessandrofelder]

I think this is ready to be merged - thanks a lot @sfmig ! - and will allow us to run our first benchmarks 🎉

As we move iteratively forwards and refactor, we should keep in mind the main user-facing purpose of this repo, which is to easily re-use and adapt the workflows.