Skip to content

Fix coverage #381

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

Merged
merged 4 commits into from
Nov 13, 2020
Merged

Fix coverage #381

merged 4 commits into from
Nov 13, 2020

Conversation

tomwhite
Copy link
Collaborator

This is to fix #380, first by causing the build to fail (by adding the --cov-fail-under pytest option) - I will then add a commit to fix the remaining coverage issues.

Copy link
Collaborator

@eric-czech eric-czech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tomwhite
Copy link
Collaborator Author

As expected this fails with

FAIL Required test coverage of 100% not reached. Total coverage: 98.79%

@tomwhite
Copy link
Collaborator Author

Thanks @eric-czech, do the fixes look OK too?

@@ -169,7 +169,7 @@ def _vcfzarr_to_dataset(
if "variants/ID" in vcfzarr:
variants_id = da.from_zarr(vcfzarr["variants/ID"]).astype(str)
else:
variants_id = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not worth testing this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored to cover this case (see pystatgen/sgkit@be7f5be)

@eric-czech
Copy link
Collaborator

Thanks @eric-czech, do the fixes look OK too?

Yep, the new tests look good to me.

Copy link
Collaborator

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov-io
Copy link

codecov-io commented Nov 13, 2020

Codecov Report

Merging #381 (be7f5be) into master (f1cfd17) will increase coverage by 4.84%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #381      +/-   ##
===========================================
+ Coverage   95.15%   100.00%   +4.84%     
===========================================
  Files          31        31              
  Lines        2334      2223     -111     
===========================================
+ Hits         2221      2223       +2     
+ Misses        113         0     -113     
Impacted Files Coverage Δ
sgkit/distance/metrics.py 100.00% <ø> (+78.78%) ⬆️
sgkit/io/bgen/__init__.py 100.00% <ø> (+50.00%) ⬆️
sgkit/io/plink/__init__.py 100.00% <ø> (+50.00%) ⬆️
sgkit/io/vcf/__init__.py 100.00% <ø> (+45.45%) ⬆️
sgkit/stats/aggregation.py 100.00% <ø> (+12.22%) ⬆️
sgkit/stats/popgen.py 100.00% <ø> (+27.17%) ⬆️
sgkit/utils.py 100.00% <ø> (+3.44%) ⬆️
sgkit/variables.py 100.00% <ø> (+3.36%) ⬆️
sgkit/io/utils.py 100.00% <100.00%> (+1.29%) ⬆️
... and 12 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 f1cfd17...be7f5be. Read the comment docs.

@jeromekelleher
Copy link
Collaborator

Two approvals here @tomwhite, so merge as you see fit!

@tomwhite tomwhite added the auto-merge Auto merge label for mergify test flight label Nov 13, 2020
@mergify mergify bot merged commit 9c24b3c into sgkit-dev:master Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Auto merge label for mergify test flight
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coverage is less than 100%
4 participants