Skip to content

Conversation

@kingbuzzman
Copy link
Contributor

Depends on:

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances test coverage tracking by tagging coverage contexts per OS/Python version, updating dependencies and CI workflows to collect and combine coverage artifacts, and adding a coverage configuration file.

  • Introduce an autouse pytest fixture to switch coverage contexts based on OS, Python version, and test name.
  • Add pytest-cov to test dependencies and modify GitHub Actions to run pytest with coverage, upload per-matrix artifacts, and aggregate them in a separate job.
  • Include a .coveragerc with coverage settings for sources, reports, and HTML output.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/conftest.py Added an autouse fixture that tags and switches coverage contexts for each test based on OS and Python version.
setup.py Included pytest-cov in the test extras.
.github/workflows/test.yml Updated test steps to include coverage flags; added artifact upload and a new coverage job to combine results.
.coveragerc Added coverage configuration for run, report, and HTML sections.
Comments suppressed due to low confidence (3)

.github/workflows/test.yml:92

  • The geekyeggo/delete-artifact action deletes remote artifacts from GitHub rather than cleaning up local files. Replace this with a local cleanup command, e.g., run: rm -rf downloaded_artifacts, to avoid impacting shared artifact storage.
      - name: Clean up temporary artifacts

tests/conftest.py:7

  • [nitpick] Naming the fixture cov shadows pytest-cov's built-in fixture and may confuse readers. Consider renaming it to e.g. coverage_context.
def cov(cov, request):

tests/conftest.py:6

  • The new coverage-context switching logic isn’t directly covered by any tests. Consider adding a unit test to verify that cov.switch_context is called with the expected context string.
@pytest.fixture(autouse=True)

if not cov:
return

sys_context = "{}-py{}.{}".format(platform.system(), sys.version_info.major, sys.version_info.minor)
Copy link

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

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

[nitpick] For improved readability and consistency with modern Python, consider using an f-string here instead of str.format().

Suggested change
sys_context = "{}-py{}.{}".format(platform.system(), sys.version_info.major, sys.version_info.minor)
sys_context = f"{platform.system()}-py{sys.version_info.major}.{sys.version_info.minor}"

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to work on 3.6! Blame @bw2 🥲

@bw2 bw2 self-requested a review June 16, 2025 01:44
Copy link
Owner

@bw2 bw2 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@bw2
Copy link
Owner

bw2 commented Jun 16, 2025

@kingbuzzman @tristanlatr any reason not to merge this?

@kingbuzzman
Copy link
Contributor Author

kingbuzzman commented Jun 16, 2025

any reason not to merge this?

Yes. Because it doesn't work 🤦‍♂️🤣

While it looks amazing on paper, the context is being ignored in CI and well, it just strips it and gives you the regular "path/file.py::TestClass::test_func" -- still working on it, hence the "draft" 🤣

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants