-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add new INI config key 'required_plugins' that defines a list of plugins that must be present for a run of pytest #7330
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
Changes from all commits
2a3c216
f760b10
3f6b3e7
f1746c5
42deba5
d2bb67b
13add4d
95cb7fb
c18afb5
57415e6
2c8e356
1474f24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
New ``required_plugins`` configuration option allows the user to specify a list of plugins required for pytest to run. An error is raised if any required plugins are not found when running pytest. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -952,6 +952,12 @@ def _initini(self, args: Sequence[str]) -> None: | |
self._parser.extra_info["inifile"] = self.inifile | ||
self._parser.addini("addopts", "extra command line options", "args") | ||
self._parser.addini("minversion", "minimally required pytest version") | ||
self._parser.addini( | ||
"required_plugins", | ||
"plugins that must be present for pytest to run", | ||
type="args", | ||
default=[], | ||
) | ||
self._override_ini = ns.override_ini or () | ||
|
||
def _consider_importhook(self, args: Sequence[str]) -> None: | ||
|
@@ -1035,7 +1041,8 @@ def _preparse(self, args: List[str], addopts: bool = True) -> None: | |
self.known_args_namespace = ns = self._parser.parse_known_args( | ||
args, namespace=copy.copy(self.option) | ||
) | ||
self._validatekeys() | ||
self._validate_plugins() | ||
self._validate_keys() | ||
if self.known_args_namespace.confcutdir is None and self.inifile: | ||
confcutdir = py.path.local(self.inifile).dirname | ||
self.known_args_namespace.confcutdir = confcutdir | ||
|
@@ -1078,12 +1085,33 @@ def _checkversion(self): | |
) | ||
) | ||
|
||
def _validatekeys(self): | ||
def _validate_keys(self) -> None: | ||
for key in sorted(self._get_unknown_ini_keys()): | ||
message = "Unknown config ini key: {}\n".format(key) | ||
if self.known_args_namespace.strict_config: | ||
fail(message, pytrace=False) | ||
sys.stderr.write("WARNING: {}".format(message)) | ||
self._warn_or_fail_if_strict("Unknown config ini key: {}\n".format(key)) | ||
|
||
def _validate_plugins(self) -> None: | ||
required_plugins = sorted(self.getini("required_plugins")) | ||
if not required_plugins: | ||
return | ||
|
||
plugin_info = self.pluginmanager.list_plugin_distinfo() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason to only include setuptools plugins here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn’t just setup tools it is all plugins, but with the proper distribution info. Correct me if I’m wrong @nicoddemus There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolving as above. Please reopen if I’m wrong There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point @bluetech. Should we consider somehow plugins specified via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to add support for -p and PYTEST_PLUGINS in a follow up PR or does that feel like we’re submitting a half complete feature @nicoddemus @bluetech ( asking since I'm not familiar with the usage of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't checked what each means, but just from perusing the I guess it depends on why the code uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was discussed here: #7330 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From reading the help, I think we should defer
The same goes for the environment variable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, not sure this makes sense when used with |
||
plugin_dist_names = [dist.project_name for _, dist in plugin_info] | ||
|
||
missing_plugins = [] | ||
for plugin in required_plugins: | ||
if plugin not in plugin_dist_names: | ||
missing_plugins.append(plugin) | ||
|
||
if missing_plugins: | ||
fail( | ||
gnikonorov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"Missing required plugins: {}".format(", ".join(missing_plugins)), | ||
pytrace=False, | ||
) | ||
|
||
def _warn_or_fail_if_strict(self, message: str) -> None: | ||
if self.known_args_namespace.strict_config: | ||
fail(message, pytrace=False) | ||
sys.stderr.write("WARNING: {}".format(message)) | ||
|
||
def _get_unknown_ini_keys(self) -> List[str]: | ||
parser_inicfg = self._parser._inidict | ||
|
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.
I'd omit these two lines as they're not really necessary.
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.
This was intentional. The point is to not fetch plugin info if it’s not necessary. The function is useless if there are no required plugins, so we exit
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.
I don't think this optimization is worth it, the code below doesn't do anything expensive. I think it is better to avoid special cases when they are not necessary. But it's OK if you want to keep it.
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 call to get dist info is expensive ( as per @nicoddemus’ earlier comment )
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.
Yeah I was under that impression, but I was wrong, sorry for not doing the proper research before.
Regardless I think the change is harmless and particularly I would prefer to bail out early. 😁
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.
+1 on bailing early. It’s unnecessary otherwise. We’re not writing high frequency trading software but at the same time we should do basic optimizations
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 main thing is that the condition makes it look like a Falsey
required_plugins
has a special meaning. If it is removed, the reader doesn't need to figure out that it doesn't.But just in general, if I see e.g.
sum
implemented like this:that seems redundant...
Uh oh!
There was an error while loading. Please reload this page.
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.
I mean, technically speaking a falsey
required_plugins
means to the function and the reader of the function "nothing important to do here now, just carry on"In this case we also don't have to assemble plugin_dist_names
plugin_dist_names = [dist.project_name for _, dist in plugin_info]
which we still would if we don't bail earlyAlso, doing the None check informs future readers that this item is not guaranteed to be populated which isn’t obvious otherwise