-
Notifications
You must be signed in to change notification settings - Fork 346
Attempt to make codecov less important #1203
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?
Changes from 52 commits
4e57e34
b4b9174
d85fe25
4674b61
8ee5187
e05f94e
ed346b6
9faec5c
73883c5
a0c7a97
71151c2
bb091a5
ab643df
ce156be
3dadd67
9598148
b2e9a9d
454fc18
574dea3
5f92bc2
0dff300
f9bd682
77a07e2
ef377e5
d795df0
9a1375a
48e6ea9
a36659e
45cb413
9f9cf83
9bff05a
a5d66c7
1ffc476
186dcc4
e3a6ea1
febb65c
a946de1
e38450f
917a326
a890bd3
9bcbcd8
3f519be
075a8a9
5ee4928
78e77c6
f8f4e3f
e188210
dfbc2eb
3faf64b
6e8b318
9958235
a44cb3a
595b61e
c0c7494
f9004d3
7eb83ca
764ae23
1b90774
66be0eb
9ea2987
faa344d
d61a45d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,8 @@ jobs: | |
timeout-minutes: 15 | ||
permissions: | ||
contents: read | ||
env: | ||
MATRIX_NAME: ${{ matrix.name }} | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: | ||
|
@@ -54,16 +56,18 @@ jobs: | |
|
||
- name: Run tox | ||
run: tox -e "${MATRIX_NAME}" | ||
kingbuzzman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
env: | ||
MATRIX_NAME: ${{ matrix.name }} | ||
|
||
- name: Report coverage | ||
- name: Prepare coverage file for upload | ||
if: contains(matrix.name, 'coverage') | ||
uses: codecov/codecov-action@v5 | ||
run: mv .coverage coverage.${MATRIX_NAME} | ||
kingbuzzman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- name: Upload temporary coverage artifact | ||
if: contains(matrix.name, 'coverage') | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
fail_ci_if_error: true | ||
files: ./coverage.xml | ||
token: ${{ secrets.CODECOV_TOKEN }} | ||
name: coverage-artifact-${{ matrix.name }} | ||
path: coverage.${{ matrix.name }} | ||
retention-days: 1 | ||
|
||
strategy: | ||
fail-fast: false | ||
|
@@ -143,11 +147,85 @@ jobs: | |
python: 'pypy3.9' | ||
allow_failure: false | ||
|
||
report-coverage: | ||
name: Report Combined Coverage | ||
runs-on: ubuntu-24.04 | ||
kingbuzzman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
needs: test | ||
if: always() | ||
permissions: | ||
contents: read | ||
actions: write | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: | ||
persist-credentials: false | ||
|
||
- uses: actions/setup-python@v5 | ||
with: | ||
python-version: '3.13' | ||
|
||
- name: Install coverage tool | ||
run: python -m pip install coverage[toml] | ||
|
||
- name: Download all coverage artifacts | ||
uses: actions/download-artifact@v4 | ||
with: | ||
path: downloaded-coverage-artifacts | ||
pattern: coverage-artifact-* | ||
|
||
- name: Combine coverage reports | ||
run: | | ||
mkdir combined_coverage_data | ||
find downloaded-coverage-artifacts -type f -name 'coverage.*' -exec cp {} combined_coverage_data/ \; | ||
coverage combine combined_coverage_data/* | ||
coverage xml | ||
coverage html | ||
coverage report --show-missing --format=markdown >> $GITHUB_STEP_SUMMARY | ||
|
||
- name: Determine retention days | ||
id: determine-retention-days | ||
run: | | ||
if [ "${GITHUB_REF}" = "refs/heads/main" ] || [[ "${GITHUB_REF}" == refs/tags/* ]]; then | ||
echo "retention_days=90" >> $GITHUB_OUTPUT | ||
else | ||
echo "retention_days=3" >> $GITHUB_OUTPUT | ||
fi | ||
env: | ||
GITHUB_REF: ${{ github.ref }} | ||
|
||
- name: Upload combined .coverage file | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: coverage-file | ||
path: .coverage | ||
retention-days: ${{ steps.determine-retention-days.outputs.retention_days }} | ||
include-hidden-files: true | ||
kingbuzzman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- name: Upload HTML coverage report | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: coverage-html-report | ||
path: htmlcov | ||
retention-days: ${{ steps.determine-retention-days.outputs.retention_days }} | ||
|
||
- name: Delete temporary coverage artifacts from run | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't really that necessary, as you already set them to expire in one day. Plus, it'd be impossible to inspect these files whenever you suspect something's wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True. But its just noise, i figure i'd leave it "clean" -- but you're not wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just that I've recently seen a DIY implementation of this causing CI stability issues due to GH API or network being flaky and causing chaos. That memory is still fresh :) But also, these are about inspectability. If you want fewer artifacts, you can run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that i like! Thanks! |
||
uses: geekyeggo/delete-artifact@f275313e70c08f6120db482d7a6b98377786765b # 5.1.0 | ||
with: | ||
name: coverage-artifact-* | ||
|
||
- name: Report coverage to Codecov | ||
uses: codecov/codecov-action@v5 | ||
with: | ||
files: ./coverage.xml | ||
token: ${{ secrets.CODECOV_TOKEN }} | ||
fail_ci_if_error: true | ||
verbose: true | ||
|
||
check: # This job does nothing and is only used for the branch protection | ||
if: always() | ||
|
||
needs: | ||
- test | ||
- report-coverage | ||
|
||
runs-on: ubuntu-24.04 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,8 +118,9 @@ branch = true | |
[tool.coverage.report] | ||
include = [ | ||
"pytest_django/*", | ||
"pytest_django_test/*", | ||
"tests/*", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's actually highly discouraged to skip measuring tests: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would have agreed with this 6 years ago, but there are so many good tools for the 2 reasons stated now that i find this moot. having a test with the same name; black, flake8, and ruff will complain about this, and will not let it pass. declaring functions that are never called inside tests; there is vulture and dead, and soon ruff too? as a general rule, i dislike having branching inside tests, so add code review to the list as a "tool". coverage is expensive, and test coverage seems.. like a lot for a little. having said that; i live by the words of michel de montaigne:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're already running coverage, regardless of whether you're recording line hits. If you want performance, stop collecting coverage 🤷♂️ The reason this is important is that this gives an indication of whether there's dead code in tests. Like things, you thought were running but aren't. Maybe, because of CI misconfiguration or a typo in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be a Codecov check that demands 100% coverage on the tests so that it's possible to reliably tell that there's no accidentally skipped tests or parts of the testing infra. Otherwise, there's a perception that things are being tested when they aren't. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're headed that direction, its anyone's guess when we arrive 😇 |
||
] | ||
kingbuzzman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
omit = [ | ||
kingbuzzman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"pytest_django/_version.py", | ||
kingbuzzman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
] | ||
skip_covered = true | ||
exclude_lines = [ | ||
|
Uh oh!
There was an error while loading. Please reload this page.