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

ci(codecov): exclude lines #2101

Merged
merged 3 commits into from
Mar 2, 2021
Merged

ci(codecov): exclude lines #2101

merged 3 commits into from
Mar 2, 2021

Conversation

Yongxuanzhang
Copy link
Contributor

@Yongxuanzhang Yongxuanzhang commented Mar 1, 2021

This ticket aims at removing the codecov detection on some specific lines, (e.g. NotImplementedError). For this error in the base class, we don't have to write tests to cover it. Exclude them will also increase the test coverage.

File introduced: .coveragerc

  1. Can we exclude these exceptions?
  2. Can we put this file in the root folder?
  3. If so, any other we codes want to exclude?

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #2101 (e1cce50) into master (360fe61) will increase coverage by 0.51%.
The diff coverage is 97.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2101      +/-   ##
==========================================
+ Coverage   89.23%   89.74%   +0.51%     
==========================================
  Files         208      208              
  Lines       11048    11034      -14     
==========================================
+ Hits         9859     9903      +44     
+ Misses       1189     1131      -58     
Flag Coverage Δ
daemon 50.83% <15.00%> (+0.01%) ⬆️
jina 90.22% <97.50%> (+0.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jina/executors/indexers/vector.py 95.68% <97.43%> (+1.24%) ⬆️
jina/__init__.py 73.49% <100.00%> (ø)
jina/helper.py 83.76% <0.00%> (-2.21%) ⬇️
jina/types/document/__init__.py 91.44% <0.00%> (-0.03%) ⬇️
jina/executors/crafters/__init__.py 100.00% <0.00%> (ø)
jina/drivers/__init__.py 93.85% <0.00%> (+0.52%) ⬆️
jina/types/querylang/queryset/lookup.py 84.95% <0.00%> (+0.74%) ⬆️
jina/executors/encoders/frameworks.py 55.00% <0.00%> (+0.90%) ⬆️
jina/peapods/pods/helper.py 98.93% <0.00%> (+1.04%) ⬆️
... and 27 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 7c50e03...25de2ec. Read the comment docs.

@@ -0,0 +1,5 @@
# .coveragerc to control coverage.py
Copy link
Member

Choose a reason for hiding this comment

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

is this supposed to affect the coverage locally or the coverage in codecov. If it is locally, u should try to create 2 reports with and without this file and see if there is difference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be " coverage in codecov".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this may not be the correct way to configure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Yongxuanzhang
Copy link
Contributor Author

Yongxuanzhang commented Mar 1, 2021

Seems like it could work, not sure if this file can work in other folders. I think it is better if I put it under .github?
@bwanglzu @JoanFM
Report
image

@github-actions
Copy link

github-actions bot commented Mar 1, 2021

Latency summary

Current PR yields:

  • 😶 index QPS at 1091, delta to last 3 avg.: +0%
  • 😶 query QPS at 18, delta to last 3 avg.: -2%

Breakdown

Version Index QPS Query QPS
current 1091 18
1.0.7 1097 18
1.0.6 1094 18

Backed by latency-tracking. Further commits will update this comment.

@Yongxuanzhang Yongxuanzhang marked this pull request as ready for review March 2, 2021 00:02
@Yongxuanzhang Yongxuanzhang requested a review from a team as a code owner March 2, 2021 00:02
@bwanglzu
Copy link
Member

bwanglzu commented Mar 2, 2021

@Yongxuanzhang i think it's better to be placed in .github, btw can you add a short description to the ticket?

@Yongxuanzhang
Copy link
Contributor Author

Yongxuanzhang commented Mar 2, 2021

@Yongxuanzhang i think it's better to be placed in .github, btw can you add a short description to the ticket?

  1. I tried to put in .github but didn't work. Or there should be some other configurations?
  2. Sure

@JoanFM
Copy link
Member

JoanFM commented Mar 2, 2021

Seems like it could work, not sure if this file can work in other folders. I think it is better if I put it under .github?
@bwanglzu @JoanFM
Report
image

I think is okey wherever u want, actually codecov.yml maybe belongs in root folder and not in .github?

@florian-hoenicke
Copy link
Member

I think in general, it is needed to exclude lines.
For this specific case, I think it can be tested.

with pytest.raises(NotImplementedError):
    obj.my_method()

@bwanglzu
Copy link
Member

bwanglzu commented Mar 2, 2021

@florian-hoenicke I personally think it makes no sense to unit test abstract methods, since the "real logic" will be implemented in subclasses.

Copy link
Member

@bwanglzu bwanglzu left a comment

Choose a reason for hiding this comment

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

LGTM, let's see how other colleagues think about it.

@Yongxuanzhang
Copy link
Contributor Author

LGTM, let's see how other colleagues think about it.

Sure! I will leave it to receive more comments.

@JoanFM
Copy link
Member

JoanFM commented Mar 2, 2021

LGTM, let's see how other colleagues think about it.

Sure! I will leave it to receive more comments.

To me is good. If u find is better to keep codecov.yml in the root of the project feel free to open a PR

@JoanFM JoanFM merged commit 0ecfffa into master Mar 2, 2021
@JoanFM JoanFM deleted the ci-update-codecov branch March 2, 2021 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants