Add new INI config key 'required_plugins' that defines a list of plugins that must be present for a run of pytest #7330

gnikonorov · 2020-06-06T15:38:42Z

Changes:

Introduce a new config key called require_plugins that is a space separated list of plugins required for pytest to run
Make the --strict-config flag ( added here treat these warnings as errors instead

gnikonorov · 2020-06-06T15:48:44Z

Hey @RonnyPfannschmidt - I think we have a flaky test for py36. What's the process for dealing with it? Looking at the logs I don't think it's related to my change

Also, I ran tox -e py36 locally on my machine ( I'm on an Ubuntu ), and all seemed to pass

(.venv) gnikonorov:pytest [issue_7305] $ cat /proc/version
Linux version 4.15.0-34-generic (buildd@lgw01-amd64-047) (gcc version 7.3.0 (Ubuntu 7.3.0-16ubuntu3)) #37-Ubuntu SMP Mon Aug 27 15:21:48 UTC 2018
(.venv) gnikonorov:pytest [issue_7305] $

RonnyPfannschmidt · 2020-06-06T17:06:29Z

I retried the build

gnikonorov · 2020-06-06T17:20:37Z

Thanks @RonnyPfannschmidt - looks like that fixed the build ( fingers crossed for no flaky tests in py37 )

nicoddemus

Hi @gnikonorov,

Thanks a lot for the contribution!

Everything looks good to me, left a couple of suggestions regarding documentation though. 👍

changelog/7305.feature.rst

doc/en/reference.rst

gnikonorov · 2020-06-07T02:36:22Z

Thanks for the review, @nicoddemus! I updated the documentation like you suggested

gnikonorov · 2020-06-07T17:04:59Z

Friendly ping for a merge @nicoddemus, @RonnyPfannschmidt. Or, should I merge myself now that I have the ability to and this has been approved?

src/_pytest/config/__init__.py

RonnyPfannschmidt · 2020-06-07T17:50:59Z

Sorry for the delay, I'm moving

gnikonorov · 2020-06-07T18:18:18Z

No worries @RonnyPfannschmidt, hope your move goes smoothly. Looks like py36 is flaky on the same tests as last time

changelog/7305.feature.rst

doc/en/reference.rst

gnikonorov · 2020-06-07T19:39:24Z

( should be ) good to merge now @nicoddemus

gnikonorov · 2020-06-08T05:09:27Z

The failure seems to be the same unrelated flaky one from before. @RonnyPfannschmidt's re-run of the build 'fixed' it last time

gnikonorov · 2020-06-09T01:10:14Z

Friendly bump for a test rerun and/or merge @nicoddemus

nicoddemus

Oh my, sorry, these last comments have been left "Pending" here in GitHub, my bad. 🙄

src/_pytest/config/__init__.py

doc/en/reference.rst

nicoddemus

As discussed:

Use dist-info instead of plugin/module name
Missing plugins should stop pytest immediately, showing all plugins which are not installed.
Change option to required_plugins

gnikonorov · 2020-06-10T04:45:23Z

Thanks for the review and thoughtful feedback. Updated the PR @nicoddemus and @RonnyPfannschmidt

gnikonorov · 2020-06-10T12:48:53Z

#7346 made to track @RonnyPfannschmidt's suggestion of specifying version constraints

nicoddemus

Thanks a lot @gnikonorov!

I've left a few minor suggestions, otherwise LGTM! 👍

src/_pytest/config/__init__.py

gnikonorov · 2020-06-10T23:23:48Z

Thank you @nicoddemus! Looking forward to getting this in and starting on the followup task

changelog/7305.feature.rst

doc/en/reference.rst

bluetech · 2020-06-11T20:27:11Z

src/_pytest/config/__init__.py

+        if not required_plugins:
+            return


I'd omit these two lines as they're not really necessary.

Suggested change

if not required_plugins:

return

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

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.

The call to get dist info is expensive ( as per @nicoddemus’ earlier comment )

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. 😁

+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

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:

def sum(xs): if not xs: return 0 s = 0 for x in xs: s += x return s

that seems redundant...

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 early

Also, doing the None check informs future readers that this item is not guaranteed to be populated which isn’t obvious otherwise

bluetech · 2020-06-11T20:29:04Z

src/_pytest/config/__init__.py

+        if not required_plugins:
+            return
+
+        plugin_info = self.pluginmanager.list_plugin_distinfo()


Any particular reason to only include setuptools plugins here?

This isn’t just setup tools it is all plugins, but with the proper distribution info. Correct me if I’m wrong @nicoddemus

Resolving as above. Please reopen if I’m wrong

list_plugin_distinfo only lists setuptools plugins, at least this is what I understand from the pluggy code. That might be what we want here, not sure myself, but unresolving so can figure it out.

Good point @bluetech.

Should we consider somehow plugins specified via -p and PYTEST_PLUGINS? How would that look like?

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 -p and PYTEST_PLUGINS

I haven't checked what each means, but just from perusing the PlugingManager code, list_name_plugin() seems good?

I guess it depends on why the code uses dist.project_name instead of the name the plugin was registered with? (Sorry if was discussed before)

This was discussed here: #7330 (comment)

From reading the help, I think we should defer -p if we add it at all. It sounds like a user is intentionally fiddling around with the run

-p name early-load given plugin module name or entry point (multi-allowed).

The same goes for the environment variable PYTEST_PLUGINS. I'm not opposed to adding them in, but to me it really sounds like they're a different use case than this and would warrant their own PR

Agree, not sure this makes sense when used with -p or PYTEST_PLUGINS.

src/_pytest/config/__init__.py

Co-authored-by: Ran Benita <ran@unusedvar.com>

src/_pytest/main.py

bluetech · 2020-06-12T11:59:17Z

Sorry for being extra nitpicky @gnikonorov :)

… typo

…7305

gnikonorov · 2020-06-12T12:28:58Z

No need to be sorry at all! I appreciate the thoroughness, thank you!

gnikonorov · 2020-06-12T13:41:26Z

Friendly ping to @RonnyPfannschmidt for a review

gnikonorov · 2020-06-12T18:13:59Z

@nicoddemus and/or @bluetech if there are no more questions can we actually merge this into master? It’s blocking a few issues I want to start working on

nicoddemus

LGTM, let's wait for @bluetech and @RonnyPfannschmidt to take a final look before mergin. 👍

gnikonorov · 2020-06-12T19:19:30Z

Thanks for the thoughtful reviews @nicoddemus and @bluetech
I’ll wait for @RonnyPfannschmidt

nicoddemus · 2020-06-12T22:35:54Z

Thanks @gnikonorov!

gnikonorov added 3 commits June 6, 2020 02:38

Commit solution thus far, needs to be polished up pre PR

2a3c216

Touchup pre-PR

f760b10

update help for --strict-config

3f6b3e7

gnikonorov changed the title ~~Issue 7305~~ Add new INI config key 'require_plugins' that defines a list of plugins that must be present for a run of pytest Jun 6, 2020

Merge remote-tracking branch 'origin/master' into issue_7305

f1746c5

nicoddemus approved these changes Jun 7, 2020

View reviewed changes

changelog/7305.feature.rst Outdated Show resolved Hide resolved

doc/en/reference.rst Outdated Show resolved Hide resolved

Update documentation as suggested

42deba5

RonnyPfannschmidt reviewed Jun 7, 2020

View reviewed changes

src/_pytest/config/__init__.py Outdated Show resolved Hide resolved

validate plugins before keys in config files

d2bb67b

nicoddemus reviewed Jun 7, 2020

View reviewed changes

changelog/7305.feature.rst Outdated Show resolved Hide resolved

doc/en/reference.rst Outdated Show resolved Hide resolved

documentation fixes

13add4d

nicoddemus reviewed Jun 9, 2020

View reviewed changes

src/_pytest/config/__init__.py Outdated Show resolved Hide resolved

src/_pytest/config/__init__.py Outdated Show resolved Hide resolved

doc/en/reference.rst Outdated Show resolved Hide resolved

nicoddemus requested changes Jun 9, 2020

View reviewed changes

review feedback

95cb7fb

gnikonorov changed the title ~~Add new INI config key 'require_plugins' that defines a list of plugins that must be present for a run of pytest~~ Add new INI config key 'required_plugins' that defines a list of plugins that must be present for a run of pytest Jun 10, 2020

nicoddemus approved these changes Jun 10, 2020

View reviewed changes

src/_pytest/config/__init__.py Outdated Show resolved Hide resolved

src/_pytest/config/__init__.py Outdated Show resolved Hide resolved

src/_pytest/config/__init__.py Outdated Show resolved Hide resolved

final touches

c18afb5

nicoddemus requested a review from RonnyPfannschmidt June 10, 2020 23:31

gnikonorov mentioned this pull request Jun 11, 2020

refactor src/_pytest/config/__init__.py to use the warnings module #7295

Closed

bluetech reviewed Jun 11, 2020

View reviewed changes

Update changelog/7305.feature.rst

57415e6

Co-authored-by: Ran Benita <ran@unusedvar.com>

bluetech reviewed Jun 12, 2020

View reviewed changes

src/_pytest/main.py Show resolved Hide resolved

gnikonorov added 2 commits June 12, 2020 08:27

rename _emit_warning_or_fail to _warn_or_fail_if_strict and fix a doc…

2c8e356

… typo

Merge branch 'issue_7305' of github.com:gnikonorov/pytest into issue_…

1474f24

…7305

nicoddemus approved these changes Jun 12, 2020

View reviewed changes

bluetech approved these changes Jun 12, 2020

View reviewed changes

RonnyPfannschmidt approved these changes Jun 12, 2020

View reviewed changes

nicoddemus merged commit aaa6f1c into pytest-dev:master Jun 12, 2020

Add new INI config key 'required_plugins' that defines a list of plugins that must be present for a run of pytest #7330

Add new INI config key 'required_plugins' that defines a list of plugins that must be present for a run of pytest #7330

Conversation

gnikonorov commented Jun 6, 2020 • edited Loading

gnikonorov commented Jun 6, 2020 • edited Loading

RonnyPfannschmidt commented Jun 6, 2020

gnikonorov commented Jun 6, 2020

nicoddemus left a comment

Choose a reason for hiding this comment

gnikonorov commented Jun 7, 2020

gnikonorov commented Jun 7, 2020

RonnyPfannschmidt commented Jun 7, 2020

gnikonorov commented Jun 7, 2020

gnikonorov commented Jun 7, 2020

gnikonorov commented Jun 8, 2020

gnikonorov commented Jun 9, 2020

nicoddemus left a comment • edited Loading

Choose a reason for hiding this comment

nicoddemus left a comment • edited Loading

Choose a reason for hiding this comment

gnikonorov commented Jun 10, 2020

gnikonorov commented Jun 10, 2020

nicoddemus left a comment

Choose a reason for hiding this comment

gnikonorov commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnikonorov Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

gnikonorov Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluetech commented Jun 12, 2020

gnikonorov commented Jun 12, 2020

gnikonorov commented Jun 12, 2020

gnikonorov commented Jun 12, 2020

nicoddemus left a comment

Choose a reason for hiding this comment

gnikonorov commented Jun 12, 2020

nicoddemus commented Jun 12, 2020

gnikonorov commented Jun 6, 2020 •

edited

Loading

gnikonorov commented Jun 6, 2020 •

edited

Loading

nicoddemus left a comment •

edited

Loading

nicoddemus left a comment •

edited

Loading

gnikonorov Jun 12, 2020 •

edited

Loading

nicoddemus Jun 12, 2020 •

edited

Loading

gnikonorov Jun 12, 2020 •

edited

Loading