-
-
Notifications
You must be signed in to change notification settings - Fork 123
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
base: main
Are you sure you want to change the base?
Conversation
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you!
Could you fix the pre-commit failures? |
There was a problem hiding this 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)
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:
Maintainers note from the action on the error: https://github.com/marocchino/sticky-pull-request-comment/tree/v2/#error-resource-not-accessible-by-integration
The alternative is to provide a |
@JelleZijlstra I think we need your decision / help here to finish this PR. |
Read and write permissions for workflows are already enabled, so this might work once we merge it into main? |
There was a problem hiding this 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?
This reverts commit 6c43f31.
There was a problem hiding this 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.
I'll do a rewrite of the PR. In short a fork can never have a PR comment on |
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 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, The new workflow consists of github actions and one variable write:
zizmor does not report a potentially dangerous However a user could store what they want in 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 |
@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! |
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 notpython -m unittest
anymore.coverage
does (currently) not installtyping_extensions
so I think that is a safepip install
(at the moment). I tried to add a weak test that assures thattyping_extensions.__file__
is indeed the one insrc/
and notsite-packages
, but that test fails on the pypi installationDo 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: