Detect and warn/ignore local python installations #2566

ghost · 2017-07-11T18:07:46Z

This should address / close ticket #2518. The behaviour implemented is as follows:

During collection, if pytest detects that the directory it is considering is sys.prefix, the directory and any tests it contains will be ignored and a helpful warning will be issued.
If --collect-in-virtualenv (a new CLI flag) is passed, pytest will collect anyways.
If the relevant path is explicitly ignored; i.e. --ignore=./.env or what have you, the warning is silenced.

I tried to write a test but couldn't quite figure out how to effectively mock sys.prefix; I'd appreciate some help there. Other feedback is also totally welcome.

RonnyPfannschmidt · 2017-07-11T18:28:52Z

hi @jmsdvl - first thanks for this piece of work, while it makes the situation better, its however not addressing #2518 since it does not detect virtualenvs and instead takes a look at the current virtualenv only

second there should be a way to unittest however it seems rather tricky with the current infrastucture - i#ll try to take a deeper look this week

ghost · 2017-07-11T18:37:55Z

@RonnyPfannschmidt is there a bulletproof way to detect a virtualenv that isn't active? I guess I could just have the code sniff around the directory structure looking for the python executable and / or a site-packages directory. Might get expensive if its run in pytest_ignore_collect though. I just wanna help out and learn your codebase ;)

RonnyPfannschmidt · 2017-07-11T18:40:11Z

@jmsdvl i would propose to check for the platform specific virtualenv activation scripts, so it would be one check per directory

@nicoddemus any oppinion?

coveralls · 2017-07-11T18:40:27Z

Coverage decreased (-0.02%) to 91.876% when pulling c2d49e3 on jmsdvl:iss2518 into d9aaab7 on pytest-dev:features.

coveralls · 2017-07-11T19:17:04Z

Coverage decreased (-0.02%) to 91.876% when pulling 676c4f9 on jmsdvl:iss2518 into d9aaab7 on pytest-dev:features.

…f a dir is a virtualenv

coveralls · 2017-07-11T20:08:18Z

Coverage decreased (-0.03%) to 91.859% when pulling b32cfc8 on jmsdvl:iss2518 into d9aaab7 on pytest-dev:features.

nicoddemus · 2017-07-11T21:36:52Z

i would propose to check for the platform specific virtualenv activation scripts,

Sounds like a good idea to me. 👍

Is there legitimate cases you guys can think of for searching for tests inside a virtual environment?

ghost · 2017-07-11T22:16:30Z

Is there legitimate cases you guys can think of for searching for tests inside a virtual environment?

@nicoddemus not really, unless you're working on the virtual environment itself or tools that heavily interact with it. But then that's why there's a CLI override option.

I implemented the check for a relevant activate script. If that, by whatever slim chance, gives false positives, it would be fairly trivial to add an extra check for another file indicative of a virtual env (say, a python binary) in the same directory as the activate script.

nicoddemus

Awesome @jmsdvl, thanks a ton for the PR.

nicoddemus · 2017-07-11T23:51:34Z

changelog/2518.feature

@@ -0,0 +1 @@
+Collection ignores the currently active Python installation by default; `--collect-in-virtualenv` overrides this behavior.


We should update the note to mention that now we detect virtual environments.

nicoddemus · 2017-07-11T23:55:33Z

changelog/2518.feature

@@ -0,0 +1 @@
+Collection ignores the currently active Python installation by default; `--collect-in-virtualenv` overrides this behavior.


Also, I suggest we add a blurb next to norecursedirs explaining that by default pytest will also skip any virtual environments, detected by their activation script, and that can be bypassed with --collect-in-virtualenv.

nicoddemus · 2017-07-11T23:56:03Z

_pytest/main.py

@@ -70,6 +70,8 @@ def pytest_addoption(parser):
    group.addoption('--keepduplicates', '--keep-duplicates', action="store_true",
        dest="keepduplicates", default=False,
        help="Keep duplicate tests.")
+    group.addoption('--collect-in-virtualenv', action='store_true',


Please add a default=False here to be explicit, otherwise I believe the default is None.

nicoddemus · 2017-07-11T23:57:15Z

_pytest/main.py

@@ -167,6 +169,17 @@ def pytest_runtestloop(session):
    return True


+def _in_venv(path):


Nice going making this a separate function. I suggest we unittest it directly by creating directories with fake activation scripts and calling it accordingly; we want to exercise it on CI to avoid someone touching it and breaking it by accident.

nicoddemus · 2017-07-12T00:02:53Z

_pytest/main.py

@@ -177,6 +190,15 @@ def pytest_ignore_collect(path, config):
    if py.path.local(path) in ignore_paths:
        return True

+    allow_in_venv = config.getoption("collect_in_virtualenv")
+    if _in_venv(path) and not allow_in_venv:


I'm not sure we should emit this warning; the common case is for users to have some virtual environment locally, so we would generate needless warnings all the time.

We probably want to make this check only if the user did not explicitly passed the path into the command line, like what we do to while collecting python test files in python.py.

IOW, pytest .env should collect everything under .env even if .env is a virtual environment directory.

nicoddemus · 2017-07-12T00:06:31Z

But then that's why there's a CLI override option.

Oh sorry I did not look at the code until now. Just playing devil's advocate there. 😁

nicoddemus · 2017-07-12T00:13:56Z

Btw,

I just wanna help out and learn your codebase ;)

Awesome, we can always use more help. Feel free to dig around, triage issues and participate on PR reviews. 👍

ghost · 2017-07-12T04:31:53Z

Requested changes, from the top:

note has been updated
docs are updated with a blurb next to norecursedirs
default and dest arguments added
there are tests for _in_venv directly and for pytest's overall behavior, including the --collect-in-virtualenv flag and its precedence (see overall behavior below)
the warning is removed
there's no need to check if the virtualenv was passed in explicitly; the whole hook is only called if it wasn't

So the overall behavior at this point is that

If a directory has an activation script in a bin directory we ignore it, unless
the user passes --collect-in-virtualenv on the CLI, unless
the directory name matches a pattern in norecursedirs (i.e. .env won't get picked up regardless of --collect-in-virtualenv because it matches .* in norecursedirs), unless
the path was given explicitly on the command line, in which case it is never ignored.

And its all tested and in the docs. 💥

coveralls · 2017-07-12T05:05:27Z

Coverage increased (+0.009%) to 91.901% when pulling 67fca04 on jmsdvl:iss2518 into d9aaab7 on pytest-dev:features.

nicoddemus · 2017-07-12T10:39:26Z

Great, thanks a lot! 🙌

John Still added 2 commits July 11, 2017 11:52

ignore the active python installation unless told otherwise

89c7358

add news item

c2d49e3

ghost changed the title ~~Iss2518~~ Detect and warn/ignore local python installations Jul 11, 2017

trim trailing ws

676c4f9

use presence of activate script rather than sys.prefix to determine i…

b32cfc8

…f a dir is a virtualenv

nicoddemus requested changes Jul 12, 2017

View reviewed changes

update docs and note; add virtualenv collection tests

67fca04

nicoddemus approved these changes Jul 12, 2017

View reviewed changes

RonnyPfannschmidt merged commit 1485a3a into pytest-dev:features Jul 13, 2017

ghost deleted the iss2518 branch July 13, 2017 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect and warn/ignore local python installations #2566

Detect and warn/ignore local python installations #2566

ghost commented Jul 11, 2017

RonnyPfannschmidt commented Jul 11, 2017

ghost commented Jul 11, 2017

RonnyPfannschmidt commented Jul 11, 2017

coveralls commented Jul 11, 2017

coveralls commented Jul 11, 2017

coveralls commented Jul 11, 2017

nicoddemus commented Jul 11, 2017

ghost commented Jul 11, 2017 •

edited by ghost

Loading

nicoddemus left a comment

nicoddemus Jul 11, 2017 •

edited

Loading

nicoddemus Jul 11, 2017

nicoddemus Jul 11, 2017

nicoddemus Jul 11, 2017

nicoddemus Jul 12, 2017

nicoddemus Jul 12, 2017

nicoddemus commented Jul 12, 2017 •

edited

Loading

nicoddemus commented Jul 12, 2017

ghost commented Jul 12, 2017

coveralls commented Jul 12, 2017

nicoddemus commented Jul 12, 2017

		@@ -0,0 +1 @@
		Collection ignores the currently active Python installation by default; `--collect-in-virtualenv` overrides this behavior.

		@@ -167,6 +169,17 @@ def pytest_runtestloop(session):
		return True


		def _in_venv(path):

Detect and warn/ignore local python installations #2566

Detect and warn/ignore local python installations #2566

Conversation

ghost commented Jul 11, 2017

RonnyPfannschmidt commented Jul 11, 2017

ghost commented Jul 11, 2017

RonnyPfannschmidt commented Jul 11, 2017

coveralls commented Jul 11, 2017

coveralls commented Jul 11, 2017

coveralls commented Jul 11, 2017

nicoddemus commented Jul 11, 2017

ghost commented Jul 11, 2017 • edited by ghost Loading

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

nicoddemus Jul 11, 2017

Choose a reason for hiding this comment

nicoddemus Jul 11, 2017

Choose a reason for hiding this comment

nicoddemus Jul 11, 2017

Choose a reason for hiding this comment

nicoddemus Jul 12, 2017

Choose a reason for hiding this comment

nicoddemus Jul 12, 2017

Choose a reason for hiding this comment

nicoddemus commented Jul 12, 2017 • edited Loading

nicoddemus commented Jul 12, 2017

ghost commented Jul 12, 2017

coveralls commented Jul 12, 2017

nicoddemus commented Jul 12, 2017

ghost commented Jul 11, 2017 •

edited by ghost

Loading

nicoddemus Jul 11, 2017 •

edited

Loading

nicoddemus commented Jul 12, 2017 •

edited

Loading