test functions for validate_mesh_structure_pairs #114
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 24 24
Lines 1943 1943
=====================================
Misses 1943 1943 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @viktorpm - this is a good start... the structure for the necessary changes is there.
I have a few comments for improvement:
- there should also be a test where
validate_mesh_structure_pairsdoesn't raise an error. - I don't understand how both these tests can pass (but they seem to)? From looking at the code in the validation function, logically, only one of them can pass? Because once an assertion error is hit, the second assertion will not be executed. I wonder whether theres a problem in the
matchargument ofpytest.raises? What am I missing? - the sentence in the "why is this PR needed" section of this PR's description is incomplete :)
There was a problem hiding this comment.
Thanks for this @viktorpm !
We're almost there - think one of the tests need a small re-think otherwise 👍
| Expected behaviour: | ||
| Currently no atlas fails the validation function this way so the [] is always empty | ||
| --> this test function should always raise an error |
There was a problem hiding this comment.
I don't understand this. Some thoughts:
- I don't think a test function should ever raise an error
pytest.raiseschecks whether a function in the source code raises an error in the expected case - it doesn't raise an error itself (the name is slightly confusing).- this is a test for the
catch_missing_structuresfunction - in this test, we should therefore pass a different
atlaswith missing structures tocatch_missing_structures- one that we expect to raise the expected error - so we can check thatcatch_missing_structuresworks as expected.
There was a problem hiding this comment.
Thanks @alessandrofelder for clarifying this! I was overcomplicating it a bit in my head :)
So can I just mock an atlas with a missing structure and pass that to the test function?
There was a problem hiding this comment.
So can I just mock an atlas with a missing structure ...
Indeed. You may not even need to use the mocking framework to do this. You can remove a key-value-pair from the atlas.structures dict.
... and pass that to the test function?
I would formulate this as "create it in the test function and pass it to the tested function".
bg_atlasgen/validate_atlases.py
Outdated
| validation_results[atlas_name].append( | ||
| (validation_function.__name__, None, str("Pass")) | ||
| ) | ||
| except AssertionError as error: | ||
| failed_validations[atlas_name].append((validation_function, error)) | ||
| validation_results[atlas_name].append( | ||
| (validation_function.__name__, str(error), str("Fail")) | ||
| ) | ||
|
|
||
| return successful_validations, failed_validations | ||
| return validation_results |
There was a problem hiding this comment.
There was a problem hiding this comment.
I see what you mean but I'm gonna need your help with this
There was a problem hiding this comment.
@alessandrofelder, can we do this together whenever you have the time?
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…test_validation.py: updated tests
for more information, see https://pre-commit.ci
…and structures separately
…es validation functions
Co-authored-by: Alessandro Felder <[email protected]>
Description
What is this PR
Why is this PR needed?
To complete
validate_mesh_structure_pairsfunction with test functions.What does this PR do?
Adds test functions for
validate_mesh_structure_pairsvalidation function invalidate_atlases.py.References
closes #111
How has this PR been tested?
Test functions have been tested manually using
BrainGlobeAtlas("allen_mouse_100um")andBrainGlobeAtlas("admba_3d_e11_5_mouse_16u")Is this a breaking change?
No
Does this PR require an update to the documentation?
No
Checklist: