Add Coverage workflow #623

Daraan · 2025-07-07T11:48:39Z

Added a coverage workflow (resolves: #520).

Add PR comment if wanted? - but needs permission, that I can likely not set up (?)
https://github.com/marketplace/actions/code-coverage-summary
(Future) Extend PR comment to be more meaningful currently reports only "src" and no specific lines.

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

…add-coverage

AlexWaygood · 2025-07-07T12:07:05Z

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).

AlexWaygood

Nice, thank you!

.github/workflows/ci.yml

src/test_typing_extensions.py

.github/workflows/ci.yml

AlexWaygood · 2025-07-07T15:47:13Z

Could you fix the pre-commit failures?

AlexWaygood

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

Daraan · 2025-07-07T16:36:14Z

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 · 2025-08-13T09:28:19Z

@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 · 2025-08-18T02:18:49Z

.github/workflows/ci.yml

+      contents: read
+      pull-requests: write
+
+    if: ${{ always() }}


Why do we need this line?

A matter of choice, I thought it might be interesting to always get the coverage output comment even if the tests failed.
The brackets {{ might not be needed on a second look. On !cancelled might even be better.

What are you thoughts? I will adjust if you feel differently (i.e. only on success).

So having this line means it runs even if a previous run failed? My thinking was just that this line sounds redundant, it seems like if True:.

yeah, this is a magic function -- docs here: https://docs.github.com/en/actions/reference/workflows-and-actions/expressions#always

we use it for similar reasons in the third-party workflow to make sure that the create-issue-on-failure job is triggered even if one of the jobs in the needs list failed (usually if a job in the needs list fails, it cancels any other jobs that list it as required):

typing_extensions/.github/workflows/third_party.yml

Lines 371 to 396 in 9810405

needs:

- pydantic

- typing_inspect

- pycroscope

- typeguard

- typed-argument-parser

- mypy

- cattrs

- sqlalchemy

if: >-

${{

github.repository == 'python/typing_extensions'

&& always()

&& github.event_name == 'schedule'

&& (

needs.pydantic.result == 'failure'

|| needs.typing_inspect.result == 'failure'

|| needs.pycroscope.result == 'failure'

|| needs.typeguard.result == 'failure'

|| needs.typed-argument-parser.result == 'failure'

|| needs.mypy.result == 'failure'

|| needs.cattrs.result == 'failure'

|| needs.sqlalchemy.result == 'failure'

)

}}

JelleZijlstra · 2025-08-18T02:20:01Z

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

Daraan

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 · 2025-08-18T10:19:17Z

.github/workflows/ci.yml

+      contents: read
+      pull-requests: write
+
+    if: ${{ always() }}


A matter of choice, I thought it might be interesting to always get the coverage output comment even if the tests failed.
The brackets {{ might not be needed on a second look. On !cancelled might even be better.

What are you thoughts? I will adjust if you feel differently (i.e. only on success).

This reverts commit 6c43f31.

JelleZijlstra

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

Daraan · 2025-08-18T13:52:10Z

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.

Daraan added 7 commits July 7, 2025 13:09

Add coverage workflow

7078342

Add coverage summary

56faf0f

add PR write permission

045b6f7

report line misses in logs

21b240c

Merge branch 'main' into add-coverage

71daf3d

Remove install check again

3b6319d

Merge remote-tracking branch 'refs/remotes/origin/add-coverage' into …

e69ace4

…add-coverage

coverage not on mypy

4384206

AlexWaygood reviewed Jul 7, 2025

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

.github/workflows/ci.yml Outdated Show resolved Hide resolved

.github/workflows/ci.yml Outdated Show resolved Hide resolved

src/test_typing_extensions.py Outdated Show resolved Hide resolved

Daraan added 4 commits July 7, 2025 15:31

Feedback; include temp files in .coverage

b852953

Merge remote-tracking branch 'upstream' into add-coverage

022d37b

check files present

9f1b3e1

Add omit to final output

4bd0aa1

AlexWaygood reviewed Jul 7, 2025

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

Daraan added 2 commits July 7, 2025 16:14

Read env

7a7d6b3

Use pyproject.toml for coverage

ba46124

AlexWaygood reviewed Jul 7, 2025

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

Daraan added 2 commits July 7, 2025 16:53

clean comment

b9613f7

Merge branch 'main' into add-coverage

525e198

Zizmor feedback: Pin actions

034ec43

AlexWaygood approved these changes Jul 7, 2025

View reviewed changes

Merge branch 'main' into add-coverage

88ba8f6

JelleZijlstra reviewed Aug 18, 2025

View reviewed changes

Daraan commented Aug 18, 2025

View reviewed changes

Test without permissions override

6c43f31

Daraan added 2 commits August 18, 2025 12:44

Merge branch 'main' into add-coverage

943aba7

Revert "Test without permissions override"

5bbafa0

This reverts commit 6c43f31.

Daraan marked this pull request as draft August 18, 2025 13:47

JelleZijlstra approved these changes Aug 18, 2025

View reviewed changes

Daraan added 4 commits August 18, 2025 15:59

rework pr comment with workflow_run

ee8f319

overwrite pr number

2757de5

na

084e39d

continue on error as v4 has a bug

6ed9889

	needs:
	- pydantic
	- typing_inspect
	- pycroscope
	- typeguard
	- typed-argument-parser
	- mypy
	- cattrs
	- sqlalchemy

	if: >-
	${{
	github.repository == 'python/typing_extensions'
	&& always()
	&& github.event_name == 'schedule'
	&& (
	needs.pydantic.result == 'failure'
	\|\| needs.typing_inspect.result == 'failure'
	\|\| needs.pycroscope.result == 'failure'
	\|\| needs.typeguard.result == 'failure'
	\|\| needs.typed-argument-parser.result == 'failure'
	\|\| needs.mypy.result == 'failure'
	\|\| needs.cattrs.result == 'failure'
	\|\| needs.sqlalchemy.result == 'failure'
	)
	}}

Uh oh!

Add Coverage workflow #623

Are you sure you want to change the base?

Add Coverage workflow #623

Conversation

Daraan commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pip install coverage & coverage -m unittest

Uh oh!

AlexWaygood commented Jul 7, 2025

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood commented Jul 7, 2025

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Daraan commented Jul 7, 2025

Uh oh!

Daraan commented Aug 13, 2025

Uh oh!

JelleZijlstra Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

Daraan Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra commented Aug 18, 2025

Uh oh!

Daraan left a comment

Choose a reason for hiding this comment

Uh oh!

Daraan Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Daraan commented Aug 18, 2025

Uh oh!

Uh oh!

Daraan commented Jul 7, 2025 •

edited

Loading

`pip install coverage` & `coverage -m unittest`