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

[MRG] move tests from test_lca into test_lca_functions #1035

Merged
merged 4 commits into from
Jun 27, 2020
Merged

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Jun 20, 2020

#1022 added a new test file, test_lca_functions.py. This PR moves non-command-line tests from test_lca.py into test_lca_functions.py, and also removes unused imports.

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?

@ctb
Copy link
Contributor Author

ctb commented Jun 20, 2020

@erikyoung85 could you review this PR when you have time?

  • look at the changed files (under "Files changed" tab for the PR), make sure it looks like only the changes described in the PR are made
  • check out the branch, makes sure the tests pass on your own machine
  • maybe see if there's anything else in test_lca.py that should be moved b/c it's not a command line or high level API test (I'm pretty sure not, but a reviewer should double check :)
  • if you have any other comments to make, make 'em :)

thanks!

@codecov
Copy link

codecov bot commented Jun 20, 2020

Codecov Report

Merging #1035 into master will increase coverage by 9.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1035      +/-   ##
==========================================
+ Coverage   83.30%   92.42%   +9.12%     
==========================================
  Files          97       72      -25     
  Lines        8749     5454    -3295     
==========================================
- Hits         7288     5041    -2247     
+ Misses       1461      413    -1048     
Flag Coverage Δ
#rusttests ?
Impacted Files Coverage Δ
src/core/src/ffi/minhash.rs
src/core/src/index/sbt/mhbt.rs
src/core/src/sketch/minhash.rs
src/core/src/index/linear.rs
src/core/src/ffi/cmd/compute.rs
src/core/src/errors.rs
src/core/tests/minhash.rs
src/core/src/index/bigsi.rs
src/core/src/sketch/nodegraph.rs
src/core/tests/signature.rs
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5c3f2e...b5f9fd5. Read the comment docs.

@erikyoung85
Copy link
Contributor

Hey Titus, I'm curious if there is any easy way to know if a function is a command-line test or not

@ctb
Copy link
Contributor Author

ctb commented Jun 21, 2020

Hey Titus, I'm curious if there is any easy way to know if a function is a command-line test or not

if it uses the runscript or run_sourmash, it's a command line test; if it's just running Python code, it's not.

in this case there's a bunch of LCA_Database tests that I left in, so it's not a clean division into command-line and not, so maybe that part of the review would be silly. 🤷‍♂️

@ctb
Copy link
Contributor Author

ctb commented Jun 26, 2020

@erikyoung85 any comments? or does everything look ok?

@ctb ctb changed the title move tests from test_lca into test_lca_functions [MRG] move tests from test_lca into test_lca_functions Jun 27, 2020
@erikyoung85
Copy link
Contributor

Looks good to me! So sorry for how long this took especially since I was away from Sourmash all of yesterday and missed your comment. I'm guessing I should leave a review?

@ctb
Copy link
Contributor Author

ctb commented Jun 27, 2020

thanks!

@ctb ctb merged commit 6a15878 into master Jun 27, 2020
@ctb ctb deleted the refactor_lca branch June 27, 2020 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants