Skip to content

ci: run coverage for the free-threaded build#4638

Merged
davidhewitt merged 1 commit into
PyO3:mainfrom
davidhewitt:free-threaded-coverage
Oct 21, 2024
Merged

ci: run coverage for the free-threaded build#4638
davidhewitt merged 1 commit into
PyO3:mainfrom
davidhewitt:free-threaded-coverage

Conversation

@davidhewitt

@davidhewitt davidhewitt commented Oct 21, 2024

Copy link
Copy Markdown
Member

This is loosely related to #4635.

From what I've worked out experimenting with those PRs, I realise that we can run coverage on the merge commit in CI. The nice thing about this is that we can add all jobs (not just build) to coverage.

Most importantly for current work, this enables code coverage for the freethreaded job. cc @ngoldbaum

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Oct 21, 2024
@davidhewitt davidhewitt force-pushed the free-threaded-coverage branch from d17cdd6 to 24ef718 Compare October 21, 2024 18:30
@codspeed-hq

codspeed-hq Bot commented Oct 21, 2024

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #4638 will degrade performances by 12.48%

Comparing davidhewitt:free-threaded-coverage (24ef718) with main (4bb4058)

Summary

❌ 1 regressions
✅ 82 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main davidhewitt:free-threaded-coverage Change
extract_hashbrown_map 18 ms 20.5 ms -12.48%

@davidhewitt davidhewitt left a comment

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.

This looks like it's worked; I'm going to merge it so that it's available for the other in-flight freethreaded PRs. Will revert / amend if there are concerns.

Comment on lines +1 to +9
# This runs as a separate job because it needs to run on the `pull_request_target` event
# in order to access the CODECOV_TOKEN secret.
#
# This is safe because this doesn't run arbitrary code from PRs.

name: Set Codecov PR base
on:
# See safety note / doc at the top of this file.
pull_request_target:

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.

cc @alex, I believe this use of pull_request_target to be safe (unlike my last time 🙈), but if you disagree, please ping and I'll revert!

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.

As long as this doesn't run code from the PR, and only runs code from main/this .yml, it's fine. That looks true here.

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.

Yep that was my understanding too 👍

@davidhewitt davidhewitt added this pull request to the merge queue Oct 21, 2024
@ngoldbaum

Copy link
Copy Markdown
Contributor

Thanks David! It'll be nice to have coverage reports on PRs for Py_GIL_DISABLED blocks.

Merged via the queue into PyO3:main with commit 133eac8 Oct 21, 2024
@davidhewitt davidhewitt deleted the free-threaded-coverage branch October 21, 2024 20:15
@ngoldbaum

Copy link
Copy Markdown
Contributor

Over on #4588 I'm seeing the coverage tests failing with a git error: fatal: refusing to fetch into branch 'refs/heads/main' checked out at '/home/runner/work/pyo3/pyo3'. I don't immediately see what's wrong...

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

Labels

CI-skip-changelog Skip checking changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants