Repository navigation
Conversation
This changeset touches many lines throughout the testsuite, but all in
service of two primary changes:
1. Introduce PipToolsCliRunner, a wrapper over click's CliRunner.
2. Convert usages of `open(...)` for file IO to use pathlib.
The CLiRunner wrapper provides nicer default behaviors, named functions
for `pip-compile` and `pip-sync`, and automatically asserts that the
exit code of a command is 0 (unless overridden with another code to
assert). For very simple cases, the CLI runner accepts a string rather
than a list of arguments, which it will pass through `shlex.split()`.
In concert, these enhancements allow usage like
result = runner.invoke(
compile_command, ["-h"], catch_exceptions=False
)
assert result.exit_code == 0, f"{result.stderr=}, {result.stdout=}"
to be rewritten as
runner.pip_compile("-h")
These simplifications accrue accross the entire testsuite for a
significant cleanup and tightening of behaviors.
The new CLI runner's enhanced behaviors reveals several flaws in specific
tests, which need tweaks and refactors to pass. Tests which have
non-zero exit codes for CLI behaviors now have to account for this. In
a couple of specific cases, tests raised exceptions which were captured
by the runner, but not properly handled. This obscured breakage caused by
improper patching/mocking. Some tests are newly sensitive to the legacy
resolver vs the modern, backtracking one, because they exhibit different
exit codes.
pyba-chronographer
Bot
added
the
bot:chronographer:provided
- Do not test `--build-option` passed to `pip-compile` -- it is not a valid install option and fails on lower pip versions. - Use pathlib path objects for path constants, fixing an improper path join.
After the CLI Runner refactor, which required fixing other tests, no test was exercising an error raising condition in the legacy resolver wrapper. Dedicated unit tests restore coverage for the resolver and the pip-compile main function.
sirosen
force-pushed
the
improve-test-cli-runner
branch
from
August 24, 2026 03:58
9bbcac9 to
06cbe80
Compare
webknjaz
reviewed
Oct 5, 2026
|
|
||
|
|
||
| @pytest.fixture | ||
| def runner(tmp_path_cwd): |
Member
There was a problem hiding this comment.
Can we have typing and docstrings for new things plz?
webknjaz
reviewed
Oct 5, 2026
| def invoke( | ||
| self, | ||
| cli_entry_point: click.BaseCommand, | ||
| args: _c.Sequence[str] | str | None = None, |
Member
There was a problem hiding this comment.
Separate pos-only from kwargs?
Suggested change
| args: _c.Sequence[str] | str | None = None, | |
| args: _c.Sequence[str] | str | None = None, | |
| /, | |
| *, |
Per review, make CLI runner method args clearly positional-only vs keyword-only, and add an annotation to the new runner fixture. Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <578543+webknjaz@users.noreply.github.com>
webknjaz
reviewed
Oct 16, 2026
Comment on lines
+10
to
+11
| repository, depcache, tmp_path | ||
| ): |
webknjaz
reviewed
Oct 16, 2026
| from piptools.resolver import LegacyResolver | ||
|
|
||
|
|
||
| def test_legacy_resolver_checks_that_deprecated_feature_is_enabled( |
Member
There was a problem hiding this comment.
are you sure it's not two tests?
Contributor
Author
There was a problem hiding this comment.
I'm not sure. 😁
I'll split it.
webknjaz
reviewed
Oct 16, 2026
|
|
||
| # having confirmed those args work, try with a repo object which does not enable | ||
| # the legacy resolver and confirm that it errors | ||
| repository.options.deprecated_features_enabled = [] |
Member
There was a problem hiding this comment.
why not set an explicit value for the test above too? seems a bit fragile w/o it
Contributor
Author
There was a problem hiding this comment.
The repository fixture looks like so:
@pytest.fixture
def repository():
return FakeRepository(
options=FakeOptions(deprecated_features_enabled=["legacy-resolver"])
)So if we make this testing more explicit, I feel like we're stepping on our own toes a bit.
Contributor
Author
There was a problem hiding this comment.
I altered it to a parametrized test, and in the course of doing that, setting the value explicitly (even though it's redundant with the fixture) made a kind of sense.
Add missing type annotations and split the two cases being checked into separate parametrized cases. This also naturally makes the positive case more explicit.
Documentation build overview
29 files changed ·
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changeset touches many lines throughout the testsuite, but all in service of two primary changes:
Introduce PipToolsCliRunner, a wrapper over click's CliRunner.
Convert usages of
open(...)for file IO to use pathlib.The CLiRunner wrapper provides nicer default behaviors, named functions for
pip-compileandpip-sync, and automatically asserts that the exit code of a command is 0 (unless overridden with another code to assert). For very simple cases, the CLI runner accepts a string rather than a list of arguments, which it will pass throughshlex.split().In concert, these enhancements allow usage like
to be rewritten as
These simplifications accrue accross the entire testsuite for a significant cleanup and tightening of behaviors.
The new CLI runner's enhanced behaviors reveals several flaws in specific tests, which need tweaks and refactors to pass. Tests which have non-zero exit codes for CLI behaviors now have to account for this. In a couple of specific cases, tests raised exceptions which were captured by the runner, but not properly handled. This obscured breakage caused by improper patching/mocking. Some tests are newly sensitive to the legacy resolver vs the modern, backtracking one, because they exhibit different exit codes.
Contributor checklist
changelog.d/(seechangelog.d/README.mdfor instructions) or the PR text says "no changelog needed".
Maintainer checklist
bot:chronographer:skiplabel.(following Semantic Versioning).