Closed timtroendle closed 4 months ago
@timtroendle Please include the reasoning behind this issue here. It would help to contextualize what this issue is targeting.
let us draw a clear distinction between unit tests and integration tests from the get go
I'm not sure your suggested split makes sense @irm-codebase. Although I guess all "model" tests are integration tests, not all script tests are unit tests. I tend to think it's too much to hope that all script tests will be unit tests. We only have the option to do thorough unit testing in the "lib" tests.
The distinction is more about testing the final built model ("model") vs testing intermediate outputs ("scripts"), right @timtroendle ?
@brynpickering I think this PR was more about how we'd like tests to be in the future. I understood from the meeting that this distinction was desired. I'm fine in accepting the PR if I was wrong in this (although I do not really see the advantage of this setup over what is there atm if that is the case...)
Thanks for clarifying @irm-codebase. I've just checked the meeting notes. Looks like we want to give the tests more of a "submodule" feel. It doesn't strike me as a decision about spliting integration and unit tests.
@timtroendle's structure makes sense in this context as tests for a "module" (right now, .smk
files) are then stored in their own directory (under tests/scripts/[my-module]
, matching scripts/[my-module]
).
Still, it would be good to be able to run tests from module-specific rules in each Snakefile. So rules/heat.smk
has a test_heat
rule, which then calls tests/scripts/heat/test_runner.py
. However, this requires duplicating our test_runner.py
script which is already quite impenetrable and difficult to maintain... Testing in Snakemake is just a pain 😅
@brynpickering, those were exactly my thoughts. We restructured the repo a while ago and we forgot the test
folder. This tiny PR is intending to fix this and make the structure of the test folder consistent.
Surely, there's more we could do like what you suggest @brynpickering but that would require more substantial changes.
Re renaming the folders to "unit" and "integration" tests, I think you both have points. In fact, while "script" tests could potentially include integration tests, too, they are all unit tests right now. Both names would be fine for me.
Still, it would be good to be able to run tests from module-specific rules in each Snakefile. So
rules/heat.smk
has atest_heat
rule, which then callstests/scripts/heat/test_runner.py
. However, this requires duplicating ourtest_runner.py
script which is already quite impenetrable and difficult to maintain... Testing in Snakemake is just a pain 😅
@brynpickering I would only do that if we truly modularise stuff (i.e, something lives in a separate repo). Otherwise, we risk breaking pytest
recommendations, and we increase code complexity:
Snakemake DOES support pretty good testing (and they also call it integration / unit tests), but it assumes you've de-tangled workflows into separate sub-workflows.
Another option (if we do not want to move stuff out), is to ensure module-level tests are exclusive to said module, and we just have git tests actions for each... Then the "big" snakemake workflows that uses all of these only focuses on integration tests and its own (few) unit tests.
But yeah, snakemake is a pain in the neck for this stuff 😅
@irm-codebase @brynpickering what do you think of the PR eventually? Would be good to merge this soon as it's a moving target when tests get added in other PRs. :)
Based on the discussion of the dev call. This PR intends to improve the structure of the tests. Possibly, a next step could be a better modularisation. See dev call notes.
Checklist
Any checks which are not relevant to the PR can be pre-checked by the PR creator. All others should be checked by the reviewer. You can add extra checklist items here if required by the PR.