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

Fix 13314 add all plugins to pytester subprocess #13316

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Faboor
Copy link

@Faboor Faboor commented Mar 20, 2025

Closes #13314

  • Not Applicable: Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.
  • Add text like closes #XYZW to the PR description and/or commits (where XYZW is the issue number). See the github docs for more information.
  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Add yourself to AUTHORS in alphabetical order.

Running the same minimal example from #13314:

$ pytest -p pytester -rP test_pytester_minimal.py
...
________ test_pytester_minimal _________
--------- Captured stdout call ---------
running: /Users/Faboor/Projects/pytester/venv/bin/python3.12 -mpytest -p no:plug_1 -p no:plug_2 --basetemp=/private/var/folders/q0/x7bql_wd1f9gw5y8rb15tg240000gq/T/pytest-of-Faboor/pytest-11/test_pytester_minimal0/runpytest-0

Testing

Running the new test without 072bd1e using pytest -v testing/test_pytester.py::test_pytester_subprocess_with_plugins

=================================== FAILURES ===================================
_________________________________ test_plugins _________________________________

pytestconfig = <_pytest.config.Config object at 0x1071bce60>

    def test_plugins(pytestconfig):
        plugins = pytestconfig.pluginmanager.list_name_plugin()
        assert ("plug_1", None) in plugins
>       assert ("plug_2", None) in plugins
E       AssertionError: assert ('plug_2', None) in [('4402264160', <_pytest.config.PytestPluginManager object at 0x106653860>), ('pytestconfig', <_pytest.config.Config o...init__.py'>), ('main', <module '_pytest.main' from '/Users/Faboor/Projects/pytester/pytest/src/_pytest/main.py'>), ...]

test_pytester_subprocess_with_plugins.py:4: AssertionError
=========================== short test summary info ============================
FAILED test_pytester_subprocess_with_plugins.py::test_plugins - AssertionErro...
============================== 1 failed in 0.06s ===============================

and with the change:

testing/test_pytester.py::test_pytester_subprocess_with_plugins PASSED   [100%]

=================================== 1 passed in 0.47s =================================== 

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Mar 20, 2025
Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Seems reasonable! Just one question that doesn't seem clear at first sight.

@@ -1488,8 +1488,8 @@ def runpytest_subprocess(
p = make_numbered_dir(root=self.path, prefix="runpytest-", mode=0o700)
args = (f"--basetemp={p}", *args)
plugins = [x for x in self.plugins if isinstance(x, str)]
if plugins:
args = ("-p", plugins[0], *args)
for plugin in reversed(plugins):
Copy link
Member

Choose a reason for hiding this comment

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

Why reversed(...)?

Copy link
Author

@Faboor Faboor Mar 20, 2025

Choose a reason for hiding this comment

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

The plugins args get prepended to existing args, so I've used reversed(...) to make sure that the CLI args -p plugin_1 -p plugin_2 are in the same order as in pytester.plugins. I don't think it functionally matters, but just in case.

And I like the direct mapping from ["plugin_1", "plugin_2"] -> -p plugin_1 -p plugin_2 much more than the alternative.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, that makes sense now. I'm not a fan of the complexity of keeping args a tuple and all the prepending, personally I'd probably do something like (untested):

__tracebackhide__ = True
p = make_numbered_dir(root=self.path, prefix="runpytest-", mode=0o700)
pytest_args = list(self._getpytestargs())
for plugin in self.plugins:
    if isinstance(plugin, str):
        pytest_args += ["-p", plugin]
pytest_args.append("--basetemp={p}")
pytest_args += list(args)
return self.run(*pytest_args, timeout=timeout)

which seems far more readable to me.

However, you're just keeping the existing style here, so I don't think this should block this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pytester.runpytest_subprocess only adds the first plugin from pytester.plugins
2 participants