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

🏗 Add new task gulp codecov-upload to upload coverage reports #22515

Merged
merged 2 commits into from
May 28, 2019
Merged

🏗 Add new task gulp codecov-upload to upload coverage reports #22515

merged 2 commits into from
May 28, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented May 26, 2019

This PR does the following:

Partial fix for #22507
Fixes #20480

@rsimha
Copy link
Contributor Author

rsimha commented May 26, 2019

@ampproject ampproject deleted a comment from codecov bot May 27, 2019
@ampproject ampproject deleted a comment from codecov bot May 27, 2019
@ampproject ampproject deleted a comment from codecov bot May 27, 2019
Copy link
Collaborator

@estherkim estherkim left a comment

Choose a reason for hiding this comment

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

I like the bug fixes! One thought though - I don't think uploading code coverage reports should be decoupled from generating these reports. Especially if it's only something that Travis does... it's more of a util function that immediately follows the report generation, not its own gulp task. What do you think?

Also, should I wait for this PR to merge before starting #22507?

@rsimha
Copy link
Contributor Author

rsimha commented May 28, 2019

I don't think uploading code coverage reports should be decoupled from generating these reports. Especially if it's only something that Travis does... it's more of a util function that immediately follows the report generation, not its own gulp task. What do you think?

Today, code coverage is generated and uploaded separately from two places: unit and integration tests. The server then merges the sources to generate a single report. This poses a problem when one of the two test runs fails, resulting in a noisy coverage chart for our repo.

image

Decoupling uploading from generating solves this problem by making sure all sources have generated coverage data before uploading them once and for all.

Also, should I wait for this PR to merge before starting #22507?

It'll be slightly easier if this were to be merged first, but not a blocker.

@codecov
Copy link

codecov bot commented May 28, 2019

Codecov Report

Merging #22515 into master will decrease coverage by 6.94%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22515      +/-   ##
==========================================
- Coverage   85.61%   78.67%   -6.95%     
==========================================
  Files        1556      794     -762     
  Lines      145994    49485   -96509     
==========================================
- Hits       124987    38930   -86057     
+ Misses      21007    10555   -10452
Flag Coverage Δ
#integration_tests 31.8% <ø> (-9.26%) ⬇️
#unit_tests 78.05% <ø> (-7.7%) ⬇️
Impacted Files Coverage Δ
extensions/amp-story/1.0/amp-story-page.js 66.04% <0%> (-0.54%) ⬇️
extensions/amp-story/1.0/amp-story.js 77.83% <0%> (-0.13%) ⬇️
test/unit/utils/test-array.js
ads/bidtellect.js
test/unit/test-custom-element.js
...pan-zoom/0.1/test/integration/test-amp-pan-zoom.js
test/unit/test-ie-media-bug.js
...t/unit/inabox/test-inabox-frame-overlay-manager.js
...xtensions/amp-bind/0.1/test/test-bind-validator.js
...p-analytics/0.1/test/test-transport-serializers.js
... and 761 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 fad19d9...1eab084. Read the comment docs.

@rsimha
Copy link
Contributor Author

rsimha commented May 28, 2019

Merging #22515 into master will decrease coverage by 6.94%.

This is expected, since we were erroneously including ads, tests, and third party code in the coverage numbers. The new (more accurate) baseline coupled with status reports (that can be made to require a minimum patch-level coverage) will help us drive up this number.

@ampproject ampproject deleted a comment from codecov bot May 28, 2019
@ampproject ampproject deleted a comment from codecov bot May 28, 2019
@ampproject ampproject deleted a comment from codecov bot May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codecov is posting comments on PRs even though comments are disabled
4 participants