-
Notifications
You must be signed in to change notification settings - Fork 80
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
[MRG] provide "protocol" tests for Index
, CollectionManifest
, and LCA_Database
classes
#1936
Conversation
@sourmash-bio/devs curious if there is a better approach within the pytest world. |
Codecov Report
@@ Coverage Diff @@
## latest #1936 +/- ##
==========================================
+ Coverage 83.17% 83.25% +0.07%
==========================================
Files 126 126
Lines 13954 13989 +35
Branches 1910 1913 +3
==========================================
+ Hits 11606 11646 +40
+ Misses 2075 2071 -4
+ Partials 273 272 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
…o add/index_tests
Index
classesIndex
, CollectionManifest
, and LCA_Database
classes
Index
, CollectionManifest
, and LCA_Database
classesIndex
, CollectionManifest
, and LCA_Database
classes
Ready for review & merge, @sourmash-bio/devs ! |
…o add/index_tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this lgtm! ...but I can't speak to whether there are any better pytest strategies 🤷🏻♀️
Use pytest fixtures and parametrized tests to start building standardized test suites for the
Index
,CollectionManifest
, andLCA_Database
classes.This PR creates three new test files,
test_index_protocol.py
,test_manifest_protocol.py
, andtest_lca_db_protocol.py
, that use fixtures and parametrized tests to evaluate the variousIndex
,CollectionManifest
, andLCA_Database
classes. Part of the motivation is to provide a clearer and more uniform test suite for the variousSqlite*
classes being introduced in #1808 and #1933.While
Index
had a fairly well defined API already, this PR ends up defining (and refactoring) theCollectionManifest
andLCA_Database
APIs to better test and support core APIs moving forward.Both
LCA_Database
andCollectionManifest
received a lot of refactoring as a result, but no functionality was changed.In addition to creating these new tests, this PR fixes a few problems caught by the new tests:
len(...)
onSBT
to only count the number of leaves, not the number of internal nodes;sourmash lca rankinfo --minimum-num
;__len__
forLazyLoadedIndex
, and adds some correctness checks to constructor;Remaining issues:
RevIndex
doesn't obey the fullIndex
API; punted toindex.RevIndex
is not a fully implementedIndex
class #1939TODO:
sigs-to-manifest
script and the updating code there;Creates problem, and then fixes it;
Fixes #1949