Skip to content

Implement code coverage in GitHub actions #6441

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 2 commits into from
Jan 14, 2020

Conversation

nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jan 11, 2020

This overwrites the codecov.yml file in the root of the repository with
codecov-upstream.yml file (which contains the code-cov token), so PRs
and branches on the repository can upload coverage.

Suggestion from here:

#6421 (comment)

Security concerns: the token might be misused, but only to upload bogus coverage
to codecov.io, so the team believe this is harmless. If we decide to fallback
from this decision , we just need to revoke the token.

Related to #6369

(Following up from #6421, but now on my fork)

This overwrites the `codecov.yml` file in the root of the repository with
`codecov-upstream.yml` file (which contains the code-cov token)´, so PRs
and branches on the repository can upload coverage.

Suggestion from here:

pytest-dev#6421 (comment)

Security concerns: the token might be misused, but only to upload bogus coverage
to `codecov.io`, so the team believe this is harmless. If we decide to fallback
from this decision , we just need to revoke the token.

Related to pytest-dev#6369
run: "tox -e ${{ matrix.tox_env }}"

- name: Prepare coverage token
if: success() && !matrix.skip_coverage && ( github.repository == 'pytest-dev/pytest' || github.event_name == 'pull_request' )
Copy link
Member Author

Choose a reason for hiding this comment

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

@hugovk small difference here: we still upload coverage on PRs. Of course this may happen on PRs on fork -> fork, but this should be rare and not a problem.

Copy link
Member

Choose a reason for hiding this comment

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

OK, should be fine.

So, when github.event_name == 'pull_request', is it something like github.repository == 'nicoddemus/pytest'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't check, but I think so

@nicoddemus
Copy link
Member Author

nicoddemus commented Jan 11, 2020

I've tried to remove the Azure pipelines/add it back to see if I could fix the failures, but the Pipelines page no longer shows pytest-dev/pytest for me to select... weird.

Anyway, I think we can disable azure entirely now because all Windows builds will be done on GitHub actions.

Still keeping Travis for deploy, at least until #6369 is fully resolved.

@nicoddemus
Copy link
Member Author

It would be nice to merge this soon, as Azure is broken.

@blueyed
Copy link
Contributor

blueyed commented Jan 14, 2020

I do not get why we need .github/codecov-upstream.yml.
We could also skip reporting coverage for when it would not be copied, no?

@blueyed
Copy link
Contributor

blueyed commented Jan 14, 2020

Anyway, I think we can disable azure entirely now because all Windows builds will be done on GitHub actions.

Makes sense already now if it cannot be fixed.

codecov.yml Outdated
@@ -1,3 +1,5 @@
# note: `.github/codecov-upstream.yml` is basically a copy of this file, please propagate
# changes as needed
Copy link
Contributor

Choose a reason for hiding this comment

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

If (the concept of) .github/codecov-upstream.yml needs to be kept it could also just append necessary lines to this file, making this comment unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

append necessary lines to this file

What do you mean? Is there a way to merge the two files?

Copy link
Contributor

@blueyed blueyed Jan 14, 2020

Choose a reason for hiding this comment

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

cat >> codecov.yml <<EOF
codecov:
  # token from: https://codecov.io/gh/pytest-dev/pytest/settings
  # use same URL to regenerate it if needed
  token: "1eca3b1f-31a2-4fb8-a8c3-138b441b50a7"
EOF

Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @hugovk in case you want to adopt it also.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done. 👍

Used a python script so I can put docs, do assertions, etc.

@nicoddemus
Copy link
Member Author

I do not get why we need .github/codecov-upstream.yml.
We could also skip reporting coverage for when it would not be copied, no?

From here: #6421 (comment)

If you put it inline, builds on forks (even if no PR is made) will be sent to the pytest's own Codecov, which clutters up the logs somewhat, and fork users cannot see their own coverage before creating PRs.

Not sure how common it is to use codecov on forks though.

@blueyed
Copy link
Contributor

blueyed commented Jan 14, 2020

Ok, let's only add the token manually then for now.
But I suggest just appending it instead of having a separate file.

@nicoddemus
Copy link
Member Author

But I suggest just appending it instead of having a separate file.

But won't the below be a problem?

If you put it inline, builds on forks (even if no PR is made) will be sent to the pytest's own Codecov, which clutters up the logs somewhat

@blueyed
Copy link
Contributor

blueyed commented Jan 14, 2020

@nicoddemus
It's not put inline / used always, but instead of having a separate file and copying it you'll do the cat (with the same pre-conditions).

@hugovk
Copy link
Member

hugovk commented Jan 14, 2020

Appending should be fine too (with the same pre-conditions), it avoids needing to duplicate and keep the rest of the file in sync.

The catted snippet could also be in a file of you'd prefer to keep it out of the yml config.

@nicoddemus nicoddemus requested review from hugovk and blueyed January 14, 2020 10:50
Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

Looking forward to having coverage for Windows again.. 👍
Please squash.

@blueyed
Copy link
Contributor

blueyed commented Jan 14, 2020

The catted snippet could also be in a file of you'd prefer to keep it out of the yml config.

Can be even rewritten in Python.. :)

@blueyed
Copy link
Contributor

blueyed commented Jan 14, 2020

@nicoddemus
GH actions are not done for PRs to features?

@nicoddemus
Copy link
Member Author

GH actions are not done for PRs to features?

Not yet it seems. I will go ahead and merge this one.

@nicoddemus nicoddemus merged commit 00adb4e into pytest-dev:master Jan 14, 2020
@nicoddemus nicoddemus deleted the gh-actions-cov branch January 14, 2020 12:14
@nicoddemus
Copy link
Member Author

Sorry, just realized we still needed a small change in main.yaml, done it in #6458 (which only after pushing realized you had already opened #6457, sorry about the wasted time).

@blueyed
Copy link
Contributor

blueyed commented Jan 14, 2020

Please squash.

Guess I should request changes for this in the future.

@hugovk
Copy link
Member

hugovk commented Jan 14, 2020

🎉 GitHub Actions is now showing up under the builds for new commits on Codecov!

Compare to an older commit from last week:

@nicoddemus
Copy link
Member Author

Guess I should request changes for this in the future.

Sorry didn't see that; my initial take was to keep the commit, but in retrospect I agree it would have been better to squash it.

@@ -1,8 +1,7 @@
# evaluating GitHub actions for CI, disconsider failures when evaluating PRs
# evaluating GitHub actions for CI, disregard failures when evaluating PRs
#
# this is still missing:
# - deploy
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the links!

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to ping me for help/review. I'm kinda developing my own framework for GH Apps&Actions + I authored that guide & action...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do! I don't expect many problems, I've already setup deployment for a number of projects (including pytest-mock), but you never know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants