Skip to content

Implement code coverage in GitHub actions - #6441

Merged
nicoddemus merged 2 commits into
pytest-dev:masterfrom
nicoddemus:gh-actions-cov
Jan 14, 2020
Merged

Implement code coverage in GitHub actions#6441
nicoddemus merged 2 commits into
pytest-dev:masterfrom
nicoddemus:gh-actions-cov

Conversation

@nicoddemus

@nicoddemus nicoddemus commented Jan 11, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

nicoddemus commented Jan 11, 2020

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member Author

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

@blueyed

blueyed commented Jan 14, 2020

Copy link
Copy Markdown
Contributor

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

blueyed commented Jan 14, 2020

Copy link
Copy Markdown
Contributor

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.

Comment thread 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
Copy Markdown
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
Copy Markdown
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?

@blueyed blueyed Jan 14, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

blueyed commented Jan 14, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

blueyed commented Jan 14, 2020

Copy link
Copy Markdown
Contributor

@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

hugovk commented Jan 14, 2020

Copy link
Copy Markdown
Member

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 blueyed and hugovk January 14, 2020 10:50

@blueyed blueyed left a comment

Copy link
Copy Markdown
Contributor

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

blueyed commented Jan 14, 2020

Copy link
Copy Markdown
Contributor

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

blueyed commented Jan 14, 2020

Copy link
Copy Markdown
Contributor

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

@nicoddemus

Copy link
Copy Markdown
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
Copy Markdown
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

blueyed commented Jan 14, 2020

Copy link
Copy Markdown
Contributor

Please squash.

Guess I should request changes for this in the future.

@hugovk

hugovk commented Jan 14, 2020

Copy link
Copy Markdown
Member

🎉 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
Copy Markdown
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.

# evaluating GitHub actions for CI, disregard failures when evaluating PRs
#
# this is still missing:
# - deploy

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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