Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor cellfinder workflow tests #26

Closed
7 tasks done
sfmig opened this issue Oct 13, 2023 · 1 comment · Fixed by #27
Closed
7 tasks done

Refactor cellfinder workflow tests #26

sfmig opened this issue Oct 13, 2023 · 1 comment · Fixed by #27
Assignees

Comments

@sfmig
Copy link
Contributor

sfmig commented Oct 13, 2023

Suggestions for refactoring the cellfinder workflow tests, from the PR #23 review (thanks @alessandrofelder!)

  • Can we redefine the integration tests as parametrised instead, considering a range of input configs (default, fetching from GIN, fetching locally)?
  • Split the current integration tests into unit tests, each testing the individual steps (e.g., split setup into the 4 setup steps, and then test the actual workflow separately). This would allow us to remove the subprocess call and we think it would simplify the whole suite.
    • I would then use pytest's fixture caplog / capsys to assert logs
  • Make the tests more modular.
    • Separate general unit tests (parser for now) and cellfinder-specific unit tests (with modules / directories)
  • Start a utils module for all workflows (which for now would include the parser function)
    • (unit) test this separately
  • Do we need the make_config helper functions?
    • if we are doing away with the subprocess call, probably no. Instead, have a fixture that returns the config as a CellfinderConfig class instead.
    • if we do need them, can we make them smaller? can they be defined inside the relevant fixtures instead?
  • Integrate fixtures back into file with the tests' definitions, rather than in conftest.py
  • Remove autouse=True for cellfinder_cache_dir fixture: this is because autouse=True will automatically execute the fixture for every test, and we won't want e.g. brainreg tests to trigger this fixture. (Can we have autouse only at module level?)
@sfmig sfmig added the enhancement New feature or request label Oct 13, 2023
@sfmig sfmig self-assigned this Oct 13, 2023
@sfmig sfmig removed the enhancement New feature or request label Oct 13, 2023
@alessandrofelder
Copy link
Member

alessandrofelder commented Oct 13, 2023

(Can we have autouse only at module level?)

Yes - we can define it in the module instead of conftest.py I think

if an autouse fixture is defined in a test module, all its test functions automatically use it. from pytest docs

@sfmig sfmig mentioned this issue Oct 16, 2023
@alessandrofelder alessandrofelder moved this from Backlog to Planned in Core development Nov 24, 2023
@sfmig sfmig linked a pull request Dec 11, 2023 that will close this issue
@sfmig sfmig closed this as completed in #27 Dec 16, 2023
@github-project-automation github-project-automation bot moved this from Planned to Done in Core development Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants