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.

Sign up for GitHub

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

Jump to bottom

Expand on data extraction from github action runs for Coveralls #296

Merged
merged 2 commits into from
Nov 19, 2020
Merged

Expand on data extraction from github action runs for Coveralls #296

merged 2 commits into from
Nov 19, 2020

Conversation

mfherbst
Copy link
Contributor

Based on the official docs.

@mfherbst mfherbst changed the title Expand on data extraction from github action runs Expand on data extraction from github action runs for Coveralls Nov 17, 2020
@mfherbst
Copy link
Contributor Author

Regarding tests ... it's not quite clear to me how to cover this. Happy for ideas.

@codecov-io
Copy link

codecov-io commented Nov 17, 2020 •
edited
Loading

Codecov Report

Merging #296 (a142970) into master (36fc28a) will increase coverage by 0.72%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
+ Coverage   81.90%   82.63%   +0.72%     
==========================================
  Files           3        3              
  Lines         199      190       -9     
==========================================
- Hits          163      157       -6     
+ Misses         36       33       -3
Impacted Files Coverage Δ
src/codecovio.jl 76.74% <0.00%> (+0.65%) ⬆️
src/coveralls.jl 87.37% <66.66%> (+0.58%) ⬆️

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 36fc28a...a142970. Read the comment docs.

@coveralls
Copy link

coveralls commented Nov 17, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 941

  • 1 of 4 (25.0%) changed or added relevant lines in 2 files are covered.
  • 10 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.3%) to 83.108%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/coveralls.jl 1 2 50.0%
src/codecovio.jl 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
src/codecovio.jl 1 63.95%
src/coveralls.jl 9 66.02%
Totals Coverage Status
Change from base Build 932: 0.3%
Covered Lines: 123
Relevant Lines: 148
💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 937

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.06%) to 80.745%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/coveralls.jl 0 4 0.0%
Totals Coverage Status
Change from base Build 932: -2.06%
Covered Lines: 130
Relevant Lines: 161
💛 - Coveralls

3 similar comments
@coveralls
Copy link

Pull Request Test Coverage Report for Build 937

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.06%) to 80.745%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/coveralls.jl 0 4 0.0%
Totals Coverage Status
Change from base Build 932: -2.06%
Covered Lines: 130
Relevant Lines: 161
💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 937

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.06%) to 80.745%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/coveralls.jl 0 4 0.0%
Totals Coverage Status
Change from base Build 932: -2.06%
Covered Lines: 130
Relevant Lines: 161
💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 937

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.06%) to 80.745%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/coveralls.jl 0 4 0.0%
Totals Coverage Status
Change from base Build 932: -2.06%
Covered Lines: 130
Relevant Lines: 161
💛 - Coveralls

fingolfin
fingolfin approved these changes Nov 17, 2020
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thanks!

It'd be nice to have some tests for this, just like we have for the other combinations of CI provider and Codecov/Coveralls in test/runtests.jl.

But I personally wouldn't want to hold this PR hostage over this :-)

src/coveralls.jl
Comment on lines +109 to +112
event_path = open(JSON.Parser.parse, ENV["GITHUB_EVENT_PATH"])
github_pr_info = get(event_path, "pull_request", Dict())
github_pr = get(github_pr_info, "number", "")
isempty(github_pr) || (data["service_pull_request"] = github_pr)
Copy link
Member

Choose a reason for hiding this comment

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

Ooooh, that's nice, I wasn't aware of it, else I would have suggested it for PR #288 -- I wonder if perhaps you or @kyungminlee would be interested in adjusting the Codecov code path to also parse the GITHUB_EVENT_PATH JSON. In general, we try to keep the Codecov and Coveralls support in sync (but I merged PR #288 anyway, because (a) I forgot 😨 and (b) honestly, I'd rather have something that works in now than to not have it at all because it's perfect...

Anyway: very nice 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can do that.

Copy link
Contributor Author

@mfherbst mfherbst Nov 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's a bit tricky to use in general though, because for push builds you don't have that much info it seems (like I could not find how to e.g. extract the branch name in a quick search).

See https://docs.github.com/en/free-pro-team@latest/rest/reference/pulls for PRs and https://docs.github.com/en/free-pro-team@latest/rest/reference/repos#commits for push builds.

Copy link
Member

Choose a reason for hiding this comment

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

I guess one then needs to combine techniques from PR #288 and this one... Ah well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that's kind of what I did. Not sure that makes things a lot better, though.

@mfherbst mfherbst mentioned this pull request Nov 17, 2020
Merged
@fingolfin fingolfin merged commit 43af9dd into JuliaCI:master Nov 19, 2020
@mfherbst
Copy link
Contributor Author

👍 Thanks for merging.

@mfherbst mfherbst deleted the ga_coveralls branch November 20, 2020 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mfherbst @codecov-io @coveralls @fingolfin