-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: main
Are you sure you want to change the base?
Fix 13314 add all plugins to pytester subprocess #13316
Conversation
There was a problem hiding this 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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why reversed(...)
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Closes #13314
closes #XYZW
to the PR description and/or commits (whereXYZW
is the issue number). See the github docs for more information.changelog
folder, with a name like<ISSUE NUMBER>.<TYPE>.rst
. See changelog/README.rst for details.AUTHORS
in alphabetical order.Running the same minimal example from #13314:
Testing
Running the new test without 072bd1e using
pytest -v testing/test_pytester.py::test_pytester_subprocess_with_plugins
and with the change: