Skip to content
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

ci: Set up Python-focused testing infra #742

Merged
merged 18 commits into from
Jan 8, 2025

Conversation

webknjaz
Copy link
Contributor

@webknjaz webknjaz commented Jan 3, 2025

This patch integrates running pytest and pre-commit via tox.
It includes changes needed for tracking coverage automatically
for local inspection and reporting it to Codecov.

This is essentially a copy of my typical boilerplate with a
few more advanced hacks for communicating with GHA.

It can be invoked as tox, tox r, tox -qq,
tox r -e pre-commit -qq -- mypy --all-files,
tox -qq -- -vv -s tests/pytest/'tests/pytest/_cli_test.py::test_known_interrupts[ctrl-c]'
etc.

tox itself is installable or runnable ad-hoc via
pipx / uvx / uv tool install or one's favorite distro
package manager. The Python version of where it's installed is
not important. tox is not usually installed into regular
manually-managed virtualenvs. Mentioned tools maintain internal
venvs for isolating the apps and the system install would just
live side-by-side with other system stuff.

tox creates virtualenvs for its wrapped commands automatically
and keeps track of cache invalidation for them. The commands
wired via tox can either be invoked with the default arguments
or with the replacement passed to tox after the -- separator.

Furthermore, the change introduces the MyPy integration into the
pre-commit configuration and is set up in strict mode.

This patch includes a config file for MyPy and adjusts the existing
Python modules to pass the checks on every other version down
to Python 3.9.

Additionally, a set of basic unit tests is added to reach the bar
of 100% code coverage metric, which is set as expected in the
configs.

Another patch included is making stderr interceptable. Previously,
sys.stderr was exposed as a module-scoped var, while the capsys
fixture patches the stderr attribute on the sys module. So such
patching did not influence the stderr variable in the _cli module
as it remained linked to the non-patched object.
With this change, the ability to inspect printing to stderr is
recovered and is then demonstrated in the enclosed tests.

Configuration note: each underlying tool must have its configuration
in a native file, it supports and not passed via CLI arguments in
the pre-commit or tox configs. This is important because those tools
may be invoked by things other than our direct automation, like
editor/IDE plugins or other platforms/tools. In order for other
invocations to work the same, the shared configuration must be the
same. For example, if we were to disable a rule in a pylint call in
pre-commit config, it wouldn't show in CI but would break in people's
editors that just call pylint directly and show errors inline.

Put an x into the box if that apply:

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.
  • This PR is internal maintenance, it's contributor-facing and not user-facing.

Description of your changes

See above.

How can we test changes

Install tox however you like (could also optionally attempt installing tox-uv with it, in the same env) and run tox. This will execute pytest under your current Python interpreter. You can also try things like tox p -qq -e py313,py39,pre-commit, tox r -qq -e py313,py39,pre-commit, tox r -qq -e pre-commit -- mypy-py313 --all-files, tox r -qq -e pre-commit -- mypy --all-files.

Keeping this as a draft to add some GHA automation before merging.

This patch includes adding a config file for MyPy and adjusting the
existing Python modules to pass the checks on every other version down
to Python 3.9.
.codecov.yml Outdated Show resolved Hide resolved
@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 7, 2025

While there's one thing to check over in the CI, this can be generally unblocked by setting up Codecov.

@MaxymVlasov MaxymVlasov changed the title 🧪 Set up Python-focused testing infra ci: Set up Python-focused testing infra Jan 7, 2025
@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 7, 2025

@antonbabenko add check to branch protection too.

@antonbabenko
Copy link
Owner

@webknjaz Done.

image

webknjaz and others added 2 commits January 7, 2025 22:54
This patch also sets up the configs for coveragepy and Codecov.
It implements 100% coverage and requires it to remain so in a few
places.

The Codecov token is supposed to be filled out later.
@webknjaz webknjaz force-pushed the maintenance/testing-infra branch 3 times, most recently from 1045207 to a41b12f Compare January 7, 2025 22:12
webknjaz and others added 10 commits January 8, 2025 01:14
This is essentially a copy of boilerplate with a few more advanced
hacks for communicating with GHA.

It can be invoked as `tox`, `tox r`, `tox -qq`,
`tox r -e pre-commit -qq -- mypy --all-files`,
`tox -qq -- -vv -s tests/pytest/'tests/pytest/_cli_test.py::test_known_interrupts[ctrl-c]'`
etc.
Previously, `sys.stderr` was exposed as a module-scoped var, while
`capsys` patches the `stderr` attribute on the `sys` module. So such
patching did not influence the `stderr` variable in the `_cli` module
as it remained linked to the non-patched object.

With this change, the ability to inspect printing to stderr is
recovered.
They include cleaning the dist dir, producing the dists and
verifying their metadata.
This include jobs building the dists and testing out of them using
pytest in a matrix of supported runtimes. Separate jobs also run
linting.

An additional workflow invokes cron runs. It is separate because GH
is known to disable workflows occasionally. This way, if it disables
the cron workflow, it won't affect the ones running on PRs and pushes.
This is necessary for the CI to match the expected dists.
Additionally, the metadata makes the dependency resolver aware of
the supported runtimes.
The current infra does not yet allow for provisioning their deps.
This is needed for running the tests from an unpacked sdist.
This is needed for CI testing to be able to measure data from sdist.
@webknjaz webknjaz force-pushed the maintenance/testing-infra branch from 7adabdf to 542b90a Compare January 8, 2025 00:14
@webknjaz webknjaz marked this pull request as ready for review January 8, 2025 00:14
@webknjaz webknjaz requested a review from yermulnik as a code owner January 8, 2025 00:14
@webknjaz webknjaz closed this Jan 8, 2025
@webknjaz webknjaz reopened this Jan 8, 2025
This is a workaround for the Codecov action bug that seems to have a PR
fixing it but isn't yet released:
codecov/test-results-action#108.
Since the tests are being invoked from sdists and not Git, codecov
actions are too. They should have configs bundled.
@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 8, 2025

FTR, this is ready to be merged. I'll send a yamllint commit on top right after.

@MaxymVlasov
Copy link
Collaborator

MaxymVlasov commented Jan 8, 2025

Let's make codecov happy first. Or fix codecov config, as it not much sense to me to write tests for tests, if I understand correctly what it wants
image

@MaxymVlasov
Copy link
Collaborator

@yermulnik JFYI, my strategy for this and other @webknjaz PRs for now - make sure that all introduced tests passed.

Emoji and other things will be wiped out by me later, by separate PR(s). Just need to track what we want to change, to not forget about them after all needed from @webknjaz perspective settings will be merged

That's done this way because @webknjaz do not want to see changes in his branches except him, and don't want to customize much his set of common settings which he uses across multiply projects. If there will be no such incredible value that @webknjaz can provide by his work to us, I will treat this as collaboration antipattern (which I believe it is) but because all these changes not related to user-faced hooks configuration, and they overall improve workflow for Python hooks, I temporarily agree with this strategy, although I don't like this approach.

@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 8, 2025

Let's make codecov happy first. Or fix codecov config, as it not much sense to me to write tests for tests, if I understand correctly what it wants
image

@MaxymVlasov that's what I meant to show you post-merge. Note that none of these are marked as required. Also, until there's a config merged into master, Codecov doesn't really display coverage for different flags in its UI. So I wanted this merged to see what's missing. It might be better to tweak the actual config in a follow-up, when there's something to look at.

It's probably fine to drop codecov/project, but not others (this is lower because it combines MyPy type checking with pytest-produced based runtime coverage). MyPy coverage requires work, plus new to the current limitations in the ecosystem, there are corner cases that sometimes prevent 100%, and you can't do anything about them. MyPy coverage measures accuracy of typing. Specifically, whether there's anything in a type that evaluates to typing.Any. The complexity here is that this may be coming from third-party typing declarations, not from your code directly.
Hence, codecov/project/typing with its imperfect coverage value. Its threshold can be lowered, I suppose. codecov/project is a combination of coverage from MyPy and pytest, while codecov/project/typing is MyPy-only and codecov/project/lib is pytest-only under src/.

There's also a separate codecov/project/tests for pytest-produced coverage of the tests/ directory. It's an important metric that must be at 100% at all times — if it's not, this would mean dead code in tests and a false sense of security by the way of thinking that all tests run while some don't.

The patch-marked metrics only take into account what's in the diff. This is effectively more stable because they depend on a fixed number of lines, if you can't set project to 100%. The project-marked ones measure coverage of the entire project which would be flaky since the number of lines to measure changes in patches, which doesn't hit the threshold in some cases. This is why 100% is important, when possible. It should be possible for pytest, but not for MyPy.

I wouldn't touch codecov/patch for now, though. It'll be different post-merge.

There's also a 100% requirement on the coveragepy level that must be honored. It's not acceptable to lower it at all. To keep up with it, use different coveragepy-provided mechanisms for things that are known to never be covered or covered only under certain branches. Those are line exclusion patterns in the config and # pragma: no cover-like branches (see covdefaults for what's configured and available to use).

Copy link
Collaborator

@MaxymVlasov MaxymVlasov left a comment

Choose a reason for hiding this comment

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

As codecov/patch check will be fixed by different PR, as codecov settings required to be merged first, approved

@MaxymVlasov MaxymVlasov merged commit a92071c into antonbabenko:master Jan 8, 2025
62 of 63 checks passed
@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 8, 2025

As codecov/patch check will be fixed by different PR

That's the thing. It might not be necessary. Let's observe what gets reported in the other PRs before making the judgement call.

@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 8, 2025

Here's a follow-up for the missing side effect introduced here but invisible in PR CI runs: #748.

@yermulnik
Copy link
Collaborator

@yermulnik JFYI, my strategy for this and other @webknjaz PRs for now - make sure that all introduced tests passed.

Works for me 👍🏻

That's done this way because […], I temporarily agree with this strategy, although I don't like this approach.

I'd suggest to not go into this topic using such harsh statements. I do appreciate @webknjaz's contribution, a fresh breath towards Pythonizing and all details provided in conversations. We're just not that mature as Python project (and we're not a Python project).

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.

4 participants