Skip to content

Add Coverage workflow #623

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

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

Add Coverage workflow #623

wants to merge 37 commits into from

Conversation

Daraan
Copy link
Contributor

@Daraan Daraan commented Jul 7, 2025

Added a coverage workflow (resolves: #520).

Currently the logic is to upload one coverage file from each version test.

pip install coverage & coverage -m unittest

A crucial point about this workflow is that it installs and runs the test with coverage -m unittest and not python -m unittest anymore. coverage does (currently) not install typing_extensions so I think that is a safe pip install (at the moment). I tried to add a weak test that assures that typing_extensions.__file__ is indeed the one in src/ and not site-packages, but that test fails on the pypi installation


Do you prefer any changes here? e.g. run first with python when install and run again with coverage?

Does anyone know why the pypy 3.9 and 3.10 tests fail here? Some kind of code leakage, missing monkeypatch?

EDIT:

  • Changed to not run coverage on the pypi versions

@AlexWaygood
Copy link
Member

if it's a pain to run PyPy tests under coverage, I think it would be fine to do the pypy tests as a separate CI job that aren't run under coverage. None of our code is currently PyPy-specific (we had a workaround for a PyPy bug for a while but it was a tiny branch of code).

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@AlexWaygood
Copy link
Member

Could you fix the pre-commit failures?

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM if we can get the PR comment thing working! (I don't think I have the necessary permissions for that either)

@Daraan
Copy link
Contributor Author

Daraan commented Jul 7, 2025

According to https://docs.github.com/actions/how-tos/security-for-github-actions/security-guides/automatic-token-authentication, a PR from a fork can never have write access (expect the repo itself).

A relevant bit:

Finally, if the workflow was triggered by a pull request from a forked repository, and the Send write tokens to workflows from pull requests setting is not selected, the permissions are adjusted to change any write permissions to read only.


Maintainers note from the action on the error: https://github.com/marocchino/sticky-pull-request-comment/tree/v2/#error-resource-not-accessible-by-integration

check your Settings > Actions > General > Workflow permissions, and make sure to enable read and write permissions.

The alternative is to provide a GITHUB_TOKEN (defaults to ${{ github.token }}) currently.

@Daraan
Copy link
Contributor Author

Daraan commented Aug 13, 2025

@JelleZijlstra I think we need your decision / help here to finish this PR.
If we want to have a coverage report message the action needs write permission on the PR, see links above for the options.
We could also use codecov. I thought it also required a secret token, on a second look it might also without (on public repos). It comes with its own pros and cons.

@JelleZijlstra
Copy link
Member

Read and write permissions for workflows are already enabled, so this might work once we merge it into main?

Copy link
Contributor Author

@Daraan Daraan left a comment

Choose a reason for hiding this comment

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

Read and write permissions for workflows are already enabled, so this might work once we merge it into main?

I had the same thought as well. But couldn't find a definite answer yet.

On my fork I did made a PR to a branch with the coverage workflow already in it: Daraan#1 (comment) there it commented like expected on the PR.
However, if I add a PR too my main branch Daraan#2 it also comments -might be because it is my own PR 🤔?

If: Settings > Actions > General > Workflow permissions is read & write. Merge and test with a dummy PR as follow up if it really works?

@Daraan Daraan marked this pull request as draft August 18, 2025 13:47
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me! I'm happy to merge and iterate a bit more if it doesn't work properly.

@Daraan
Copy link
Contributor Author

Daraan commented Aug 18, 2025

I'll do a rewrite of the PR. In short a fork can never have a PR comment on pull_request as the permissions / secret is not accessible. I'll shift it to something that should work.

@python-cla-bot
Copy link

python-cla-bot bot commented Aug 18, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@Daraan Daraan marked this pull request as ready for review August 18, 2025 16:05
@Daraan
Copy link
Contributor Author

Daraan commented Aug 18, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

-i think i already cleaned commits need to recheck

@Daraan
Copy link
Contributor Author

Daraan commented Aug 18, 2025

I've changed the code to a working variant. However, it should be reviewed carefully as we would need to add an exception:

zizmor's warning is correct that workflow_run can be unsafe: See https://docs.zizmor.sh/audits/#dangerous-triggers

Here is a good article about it that I followed to create the new variant: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/ to avoid insecure pitfalls.

The most important bit, coverage_report.yml does not use execute user code, i.e. code checkout and code run.
The new workflow (and any modifications of it) will only be active after a merge. Any future PR modifying it should be reviewed carefully.

The new workflow consists of github actions and one variable write:

  • download a file containing the PR number and store it in the env (more below)
  • download the coverage report formatted as markdown
  • post the markdown in the respective PR number

zizmor does not report a potentially dangerous GITHUB_ENV write https://docs.zizmor.sh/audits/#github-env

However a user could store what they want in pr_number.txt. Potentially a guard checking that it only contains a small number (so it has to be a PR number) should be added.


As a conclusion to feel save would be to ask someone who is more knowledgeable about the risk to have a look and make a review

Following the guide one could rewrite the workflow to only use github scripts to extract the PR number and write the comment. This would save the call to modify GITHUB_ENV

@AlexWaygood
Copy link
Member

As a conclusion to feel save would be to ask someone who is more knowledgeable about the risk to have a look and make a review

@woodruffw -- don't suppose you could give this PR a once over from a security perspective, could you? typing_extensions is a top-10 PyPI package, so it's probably better for us to be safe than sorry here!

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.

Add test coverage monitoring
3 participants