Skip to content

chore: Use pytester fixture to test the plugin#133

Merged
edgarrmondragon merged 4 commits into
mainfrom
pytest-pytester
Jun 17, 2026
Merged

chore: Use pytester fixture to test the plugin#133
edgarrmondragon merged 4 commits into
mainfrom
pytest-pytester

Conversation

@edgarrmondragon

@edgarrmondragon edgarrmondragon commented Mar 6, 2026

Copy link
Copy Markdown
Collaborator

pytester:

Added in version 6.2.

Provides a Pytester instance that can be used to run and test pytest itself.

It provides an empty directory where pytest can be executed in isolation, and contains facilities to write tests, configuration files, and match against expected output.

testdir:

Identical to pytester, but provides an instance whose methods return legacy py.path.local objects instead when applicable.

New code should avoid using testdir in favor of pytester.

@edgarrmondragon edgarrmondragon self-assigned this Mar 6, 2026
@edgarrmondragon edgarrmondragon changed the title test: Use pytester fixture to test the plugin chore: Use pytester fixture to test the plugin Mar 6, 2026
@edgarrmondragon edgarrmondragon marked this pull request as ready for review March 6, 2026 05:51
@edgarrmondragon edgarrmondragon marked this pull request as draft March 6, 2026 05:51
@github-actions github-actions Bot added the chore label Mar 6, 2026
@github-actions github-actions Bot added chore and removed chore labels Mar 6, 2026
@github-actions github-actions Bot added chore and removed chore labels Mar 6, 2026
@edgarrmondragon edgarrmondragon marked this pull request as ready for review March 6, 2026 06:13
@edgarrmondragon edgarrmondragon enabled auto-merge (squash) March 8, 2026 14:40
auto-merge was automatically disabled April 7, 2026 19:02

Pull request was closed

@edgarrmondragon edgarrmondragon enabled auto-merge (squash) April 7, 2026 19:02
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>

@henryiii henryiii left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Here's a Claude Opus 4.8 review if that's helpful:

🤖 AI text below 🤖

All 18 tests pass on this branch (uv run pytest -q). Here's my review of PR #133 — chore: Use pytester fixture to test the plugin.

Overview

Mechanical migration of plugin_test.py from the legacy testdir/pytest.Testdir fixture to the modern pytester/pytest.Pytester fixture. Two substantive side effects beyond the rename:

  • Drops the local no_fnmatch_line helper, calling result.stderr.no_fnmatch_line(...) directly.
  • Switches from testdir.monkeypatch to the standalone monkeypatch fixture.

Code quality

  • Right call dropping the helper. Its comment said no_fnmatch_line was added to testdir in pytest 5.3.0; the project floor is pytest 7, so the shim was dead weight. RunResult.stderr.no_fnmatch_line is available well within the supported range.
  • Escape semantics preserved. The old helper appended "*" to every pattern. The inline replacements fold that * into the literal pattern correctly — e.g. "...succeed_no_output.py""...succeed_no_output.py*". Where the caller already passed a trailing * (e.g. "::*assert x += 1*"), the helper would have produced **; the new single-* form is fnmatch-equivalent, so no behavior change.
  • monkeypatch fixture swap is correct. Using the standalone fixture instead of pytester.monkeypatch is the documented pattern and keeps env setup independent of the pytester instance. No ordering concern since setenv doesn't depend on cwd.
  • Nice tightening in test_annotation_subtest: the added assert len(result.stderr.lines) == 2 makes the test verify the exact annotation count rather than just presence.
  • os import correctly retained — still used by the *_cwd tests via os.path.dirname.

Suggestions (minor / non-blocking)

  • Inconsistent idiom for the parent path. test_annotation_fail_cwd uses the cleaner str(pytester.path.parent), but test_annotation_warning_cwd still uses os.path.dirname(str(pytester.path)) for the same thing. Since pytester.path is a pathlib.Path, standardizing both on .parent would let the os import drop entirely and read more uniformly. Worth a one-line follow-up, not a blocker.

Risk / correctness

  • Low risk: pure test-infrastructure change, no plugin source touched. pytester is the actively-maintained successor to testdir, so this reduces deprecation surface. Full suite (incl. the xdist and subtest paths) passes locally.
  • Note: line 9 still uses PYTEST_VERSION/packaging (the subtest skipif), so this branch is unaffected by the parallel PR #141 that removes them — no conflict in intent, though one will need a trivial rebase after the other merges.

Verdict: Clean, low-risk modernization that's a net improvement. Approve; optionally fold in the pytester.path.parent consistency tweak before merge.

….parent)` instead of `os.path.dirname(str(pytester.path))`

Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
@edgarrmondragon

Copy link
Copy Markdown
Collaborator Author

Thanks for asking Claude for a review @henryiii!

I've addressed the suggestion.

  • Inconsistent idiom for the parent path. test_annotation_fail_cwd uses the cleaner str(pytester.path.parent), but test_annotation_warning_cwd still uses os.path.dirname(str(pytester.path)) for the same thing. Since pytester.path is a pathlib.Path, standardizing both on .parent would let the os import drop entirely and read more uniformly. Worth a one-line follow-up, not a blocker.

@edgarrmondragon edgarrmondragon merged commit 466b4e2 into main Jun 17, 2026
15 checks passed
@edgarrmondragon edgarrmondragon deleted the pytest-pytester branch June 17, 2026 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants