Skip to content

Delete many tests #652

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

Closed
wants to merge 3 commits into from
Closed

Conversation

AlexWaygood
Copy link
Member

The same as #651, but this time I'm making the PR from a branch on my fork. For security reasons, this should not trigger a comment from the bot.

(testing to see if I get a comment from the new coverage workflow when I make a PR from a branch on the upstream repo)
@AlexWaygood AlexWaygood marked this pull request as draft August 22, 2025 14:31
@AlexWaygood
Copy link
Member Author

Nice: the coverage report is posted in the logs here, but there is no comment posted, which is what we wanted: https://github.com/python/typing_extensions/actions/runs/17158064310/job/48679934651?pr=652

@AlexWaygood AlexWaygood deleted the no-comment-from-fork branch August 22, 2025 14:33
@Daraan
Copy link
Contributor

Daraan commented Aug 22, 2025

You could try to go further and get coverage <90% and test if the tests really fail :)

@AlexWaygood AlexWaygood mentioned this pull request Aug 22, 2025
2 tasks
@AlexWaygood AlexWaygood restored the no-comment-from-fork branch August 22, 2025 14:35
@AlexWaygood AlexWaygood reopened this Aug 22, 2025
@Daraan
Copy link
Contributor

Daraan commented Aug 22, 2025

Still at 91%. I think because the test file is itself included in the coverage...
Main file is down to 77%

@AlexWaygood
Copy link
Member Author

Down to 89%, and now the CI job correctly fails 🎉

But this makes me think that we should raise the threshold below which the job fails. It seems like we can delete many tests before the job starts failing, which doesn't seem great.

@AlexWaygood AlexWaygood deleted the no-comment-from-fork branch August 22, 2025 14:47
@Daraan
Copy link
Contributor

Daraan commented Aug 22, 2025

Down to 89%, and now the CI job correctly fails 🎉

But this makes me think that we should raise the threshold below which the job fails. It seems like we can delete many tests before the job starts failing, which doesn't seem great.

Thanks for testing.

Mixing:
~4000 lines in typing_extensions (priority)
~9000 lines in test_typing_extensions of which nearly all are executed skews the percentage into the positive. I would now propose to exclude the test file from the calculation.

@AlexWaygood
Copy link
Member Author

I would now propose to exclude the test file from the calculation.

I don't think we should do that -- it's way too easy to get into a situation where you accidentally don't run tests at all in Python, and including the test file in the consideration guards against that. See https://nedbatchelder.com/blog/201908/dont_omit_tests_from_coverage.html

@JelleZijlstra
Copy link
Member

If I recall correctly the current test coverage is like 97%, right? So I'd be OK with having the fail threshold at 96.

@Daraan
Copy link
Contributor

Daraan commented Aug 22, 2025

With ~3s more to the workflow we could also add an html report, which is also pretty nice for exploring via GUI (needs to be manually downloaded):
grafik

https://github.com/Daraan/typing_extensions/actions/runs/17159151595/job/48683686044

zipped html report from above's run
https://github.com/Daraan/typing_extensions/actions/runs/17159151595/artifacts/3829560437


As a follow up the report could be piped into GitHub Pages without much effort (I think). Leaving this here if someone wants to check it out:

I think it would be sufficient to extend the current ci workflow on a branches: main push event to upload the html artifact to Pages.

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

Successfully merging this pull request may close these issues.

3 participants