Skip to content

chore: bump python version used for codecov in CI #520

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 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,22 @@ jobs:
- name: List installed Python packages
run: python -m pip list
- name: Test with pytest, generate coverage report (skipping typeguard)
if: matrix.python-version == '3.8'
if: matrix.python-version == '3.9'
run: |
# https://github.com/scikit-hep/cabinetry/issues/428
pip install --upgrade typing_extensions
# skip typeguard for coverage https://github.com/agronholm/typeguard/issues/356
pytest --runslow --cov-report=xml --typeguard-packages=""
- name: Test with pytest
if: matrix.python-version != '3.8'
if: matrix.python-version != '3.9' && matrix.python-version != '3.8'
run: |
pytest --runslow
- name: Upload coverage to codecov
- name: Test with pytest for py38
if: matrix.python-version == '3.8'
run: |
# https://github.com/scikit-hep/cabinetry/issues/428
pip install --upgrade typing_extensions
pytest --runslow
- name: Upload coverage to codecov
if: matrix.python-version == '3.9'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the coverage only runs for 3.8 (and other versions run pytest without it), so this needs to change in the two places above as well. Have a look at #428, possibly this update means we can also remove the typing_extensions install there.

Copy link
Collaborator Author

@MoAly98 MoAly98 Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this

    - name: Test with pytest
      if: matrix.python-version != '3.8'
      run: |
        pytest --runslow  

stays the same, right? I am guessing the reason to only run the slow tests on py>3.8 had a reason beyond 3.8 being the oldest version supported.

Copy link
Member

@alexander-held alexander-held Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think they should all change. The logic is:

  • run pytest with coverage for a specific version (previously 3.8, now 3.9),
  • run pytest without coverage for the other versions (previously !=3.8, now !=3.9),
  • upload if on the right version (previously 3.8, now 3.9).

I'm not sure what the 3.9 CI actually uploaded now because it didn't seem to fail. That surprised me since it should not have generated a coverage report in the current state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right but then for #428 to be dropped we would need to drop 3.8 all together, because the CI will still be running that (why it currently fails)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I see now it's unrelated to coverage

uses: codecov/codecov-action@v4
with:
token: ${{ secrets.CODECOV_TOKEN }}
Expand Down
Loading