Consider plugins loaded by PYTEST_PLUGINS for assertion rewrite

[nicoddemus]

Fix #2185

[coveralls]

Coverage increased (+0.002%) to 92.819% when pulling 043aade on nicoddemus:pytest-plugins-env-rewrite into ff309b3 on pytest-dev:master.

[RonnyPfannschmidt]

this it is a behaviour change at fist glance

[RonnyPfannschmidt]

the real marking should be in the _import_plugin_specs function before the import loop but after the split

[nicoddemus]

It feels to me import_plugin is the better place for it because all other functions which import a plugin eventually call it, so don't miss marking plugins for assertion rewrite:

consider_pluginarg -> import_plugin
consider_conftest -> consider_module -> _import_plugin_specs -> import_plugin
consider_module -> _import_plugin_specs -> import_plugin

So I think this actually also fixes consider_pluginarg.

Could you provide an example were this would be a behavior change?

[RonnyPfannschmidt]

thanks for taking a deeper look, the location you mention is indeed a better place

my feat of the behaviour change has 2 parts 1. we now pass more different types trough the functions (before it was None or a string, now it is None, a string or a list)
upon investigation of course i noticed that the _import_plugin_specs does filter "false" values
we may experience misbehavior on the value "," however (which would also hold prior to the change)

[nicoddemus]

Thanks for the reply @RonnyPfannschmidt

1. we now pass more different types trough the functions (before it was None or a string, now it is None, a string or a list)

You mean the change in consider_env? I don't see a behavior change there, because it went from passing None or str to passing a list[str] to _import_plugin_specs, which was always supported by _import_plugin_specs anyway. Or do you mean that previously consider_env would always call _import_plugin_specs, even with None? I guess we can change consider_env so that it calls _import_plugin_specs even when PYTEST_PLUGINS is not defined, but it seems to me an stretch to consider this a "behavior change" (which I'm assuming you mean to put this fix into features instead of master). If that's what you mean, could you provide an example of a situation where this would affect users at all, considering that the config module is rarely (if ever) overwritten by plugins and users?

[nicoddemus]

Also, I think that all those places with handle the different ways to specify a plugin are getting unhandy to maintain.

if specs:
       specs = specs.split(',')

And:

if isinstance(plugins, str):
    plugins = [plugins]

Should we add a

def _get_plugins_as_list(specs: Union[str, None, List[str])) -> List[str]

Function and use it everywhere? Some subtle bugs have crept in because this logic is implemented everywhere, sometimes incompletely.

[RonnyPfannschmidt]

that may be necessary, and a good thing to do, we have so many inconsistencies ^^

[nicoddemus]

OK, thanks, will do it then.

And about your reservations for this being a behavior change?

[RonnyPfannschmidt]

with the right location for the marking code i no longer have any reservations at all, thanks

[nicoddemus]

Great. 👍

Implemented the change as we agreed,

[RonnyPfannschmidt]

the real marking should be in the _import_plugin_specs function before the import loop but after the split

[coveralls]

Coverage increased (+0.002%) to 92.819% when pulling 0bc8310 on nicoddemus:pytest-plugins-env-rewrite into ff309b3 on pytest-dev:master.

[coveralls]

Coverage increased (+0.002%) to 92.819% when pulling 7cd7c28 on nicoddemus:pytest-plugins-env-rewrite into ff309b3 on pytest-dev:master.

[RonnyPfannschmidt]

lovely finalization, ready to merge i presume?

[nicoddemus]

Thanks Ronny! Merging then. 😁