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

[test] Disable codecov upload on PR builds #2541

Merged
merged 3 commits into from
Dec 10, 2019
Merged

Conversation

natanlao
Copy link
Contributor

@natanlao natanlao commented Oct 22, 2019

Codecov has been returning weird results for some builds, usually
trending lower (saying that code coverage has dropped by some
ridiculous amount). I think this is because Codecov is getting
confused by all the different reports being uploaded - five from
the PR build, and another five from the branch build.

Here's an example of Codecov conflating reports from different
builds:

https://codecov.io/gh/HumanCellAtlas/data-store/commit/2fcdaa2c27de001e6a436371950d0caecdb40d3d/builds

Closes #2692

@natanlao
Copy link
Contributor Author

The expected behavior is that Codecov reports CI failure (for the linked report).

@codecov
Copy link

codecov bot commented Oct 22, 2019

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2541       +/-   ##
===========================================
+ Coverage   47.38%   78.35%   +30.97%     
===========================================
  Files          84       84               
  Lines        5960     5960               
===========================================
+ Hits         2824     4670     +1846     
+ Misses       3136     1290     -1846
Impacted Files Coverage Δ
dss/logging.py 92.5% <0%> (+2.5%) ⬆️
dss/index/es/__init__.py 75.36% <0%> (+5.79%) ⬆️
dss/subscriptions_v2/__init__.py 70.58% <0%> (+5.88%) ⬆️
dss/storage/identifiers.py 97.53% <0%> (+8.64%) ⬆️
dss/config.py 92.65% <0%> (+9.03%) ⬆️
dss/storage/files.py 100% <0%> (+10%) ⬆️
dss/api/collections.py 36.87% <0%> (+10.62%) ⬆️
dss/util/retry.py 96.11% <0%> (+10.67%) ⬆️
dss/error.py 83.33% <0%> (+13.63%) ⬆️
dss/operations/__init__.py 89.09% <0%> (+16.36%) ⬆️
... and 42 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 d2b6894...406e24b. Read the comment docs.

@natanlao
Copy link
Contributor Author

I'm not entirely convinced that this fix works, I'm going to leave this PR open to do some more testing over the next week or so.

@natanlao
Copy link
Contributor Author

Exhibit B

Codecov has been returning weird results for some builds, usually
trending lower (saying that code coverage has dropped by some
ridiculous amount). I think this is because Codecov is getting
confused by all the different reports being uploaded - five from
the PR build, and another five from the branch build.

Here's an example of Codecov conflating reports from different
builds:

https://codecov.io/gh/HumanCellAtlas/data-store/commit/2fcdaa2c27de001e6a436371950d0caecdb40d3d/builds
@@ -0,0 +1,3 @@
codecov:
notify:
after_n_builds: 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the 5 come from? The codecov.io documentation of after_n_builds says it should be set greater than the number of reports uploaded per commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read

Generally, you should set after_n_builds equal to the number of reports you upload per commit.

There are five codecov reports (one per job) uploaded per build, so I think this is right. One part I am unclear of is if it refers to number of reports uploaded per commit or per build - if it is the former, then, were we not to disable codecov on one of two builds per PR, after_n_builds should be set to ten. But I think with these changes it might not matter...

Copy link
Contributor

Choose a reason for hiding this comment

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

Will keep an eye out for whether the reported statistics improve - if not we can try bumping from 5 to 10.

@chmreid
Copy link
Contributor

chmreid commented Nov 5, 2019

Hmm, the old codecov reports that demonstrated the improved behavior seem to have disappeared

@chmreid chmreid added the DevOps Tasks that related to the operation of the DCP label Nov 26, 2019
@chmreid chmreid added this to the Q4 2019 Milestone 3 milestone Dec 10, 2019
@chmreid
Copy link
Contributor

chmreid commented Dec 10, 2019

We revisited this during standup today, I think we're going to merge this in and keep an eye on the codecov reports to see if there is any improvement.

@chmreid chmreid merged commit df70c61 into master Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DevOps Tasks that related to the operation of the DCP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve codecov reports
2 participants