Improve handling of pyargs #3010

cryvate · 2017-12-07T11:47:33Z

The conversion of pyargs to paths is improved here:

patch pkgutil to fix issue pyargs with subpackage breaks fixture (conftest.py?) discovery #2985 in python2.7.

Note that this might not fix the issue in python 3.0-3, as pkgutil.find_loader still worked like in python2.7 but as it is not supported I did not look into it.

I couldn't just use the backport of importlib, as it does not have importlib.find_loader or importlib.util.find_spec which is what is needed. The current (3.4+) pkgutil.find_loader uses importlib.util.find_spec under the hood, instead of parts of pkgutil.

cryvate · 2017-12-07T13:05:28Z

testing/acceptance_test.py

+
+        # The structure of the test directory is now:
+        # .
+        # ├── local


I tried to keep close to the actual issue, and kept the virtualenv structure.

coveralls · 2017-12-07T13:18:36Z

Coverage decreased (-0.3%) to 92.416% when pulling 4e5344d on cryvate:fix-issue-2985 into 88f2cc9 on pytest-dev:master.

cryvate · 2017-12-07T13:26:26Z

testing/acceptance_test.py

@@ -645,6 +647,69 @@ def join_pythonpath(*dirs):
            "*1 passed*"
        ])

+    @pytest.mark.skipif(not hasattr(os, "symlink"), reason="requires symlinks")


This assumes if the python/os supports symlinks, so does the filesystem the temporary directory is created. This is not a given, but as this is in testing only, and generally this is the case (both in CI and locally) in most workflows, I think this is good enough.

I agree 😁

coveralls · 2017-12-07T13:31:00Z

Coverage decreased (-0.1%) to 92.57% when pulling 4e5344d on cryvate:fix-issue-2985 into 88f2cc9 on pytest-dev:master.

coveralls · 2017-12-07T13:49:03Z

Coverage decreased (-0.1%) to 92.57% when pulling df6885d on cryvate:fix-issue-2985 into 88f2cc9 on pytest-dev:master.

coveralls · 2017-12-07T15:42:34Z

Coverage decreased (-0.1%) to 92.57% when pulling b62fd79 on cryvate:fix-issue-2985 into 88f2cc9 on pytest-dev:master.

nicoddemus

Great work! Thanks for debugging and studying the underlying issue, it is quite a ride.

I made a few comments, please take a look at them when you have the chance.

nicoddemus · 2017-12-11T23:32:55Z

_pytest/main.py

+                    return None
+                return pkgutil.ImpLoader(fullname, file, filename, etc)
+
+            pkgutil.ImpImporter.find_module = find_module_patched


I'm a little concerned about patching this globally like it is done here... who knows what we might break by patching this.

I would rather be conservative and patch this only while we are calling find_loader in the lines below this one. I think a nice way to implement this would be by using a contextmanager:

try: with _patched_find_module(): loader = pkgutil.find_loader(x) except ImportError:

And _patched_find_module can be implemented like so:

@contextlib.contextmanager def _patched_find_module(): """ ... """ if six.PY2: # python 3.4+ uses importlib instead def find_module_patched(self, fullname, path=None): subname = fullname.split(".")[-1] if subname != fullname and self.path is None: return None if self.path is None: path = None else: path = [self.path] try: file, filename, etc = pkgutil.imp.find_module(subname, path) except ImportError: return None return pkgutil.ImpLoader(fullname, file, filename, etc) old_find_module = pkgutil.ImpImporter.find_module pkgutil.ImpImporter.find_module = find_module_patched try: yield finally: pkgutil.ImpImporter.find_module = old_find_module else: yield

We can then even use _patched_find_module()'s docstring to better explain what's going on.

nicoddemus · 2017-12-11T23:33:50Z

testing/acceptance_test.py

@@ -645,6 +647,69 @@ def join_pythonpath(*dirs):
            "*1 passed*"
        ])

+    @pytest.mark.skipif(not hasattr(os, "symlink"), reason="requires symlinks")


I agree 😁

nicoddemus · 2017-12-11T23:39:44Z

testing/acceptance_test.py

+        # The structure of the test directory is now:
+        # .
+        # ├── local
+        # │   └── lib -> ../world


I think this comment is incorrect, there's no world directory being created

nicoddemus · 2017-12-11T23:40:04Z

testing/acceptance_test.py

+        #         └── bar
+        #             ├── __init__.py
+        #             ├── conftest.py
+        #             └── test_world.py


Ditto here, the code above is creating test_bar.py

nicoddemus · 2017-12-11T23:40:49Z

testing/acceptance_test.py

+        #             └── test_world.py
+
+        def join_pythonpath(*dirs):
+            cur = py.std.os.environ.get('PYTHONPATH')


You can use os.environ directly, we are planning on dropping py.std usage eventually

cryvate · 2017-12-12T08:30:47Z

_pytest/main.py

        except ImportError:
            return x
        if loader is None:
            return x
        # This method is sometimes invoked when AssertionRewritingHook, which
        # does not define a get_filename method, is already in place:
        try:
-            path = loader.get_filename(x)
+            with _patched_find_module():


I'm not 100% sure why it is needed here, but I do know the tests fail if you don't have patching in both places.

…ager.

coveralls · 2017-12-12T08:57:44Z

Coverage decreased (-0.2%) to 92.582% when pulling b462110 on cryvate:fix-issue-2985 into 5c6d773 on pytest-dev:master.

coveralls · 2017-12-12T09:10:44Z

Coverage decreased (-0.2%) to 92.582% when pulling 3ca1e4b on cryvate:fix-issue-2985 into 5c6d773 on pytest-dev:master.

coveralls · 2017-12-12T09:30:56Z

Coverage decreased (-0.2%) to 92.582% when pulling 3ca1e4b on cryvate:fix-issue-2985 into 5c6d773 on pytest-dev:master.

nicoddemus · 2017-12-12T09:49:41Z

_pytest/main.py

@@ -728,17 +730,57 @@ def _tryconvertpyarg(self, x):
        """Convert a dotted module name to path.

        """
-        import pkgutil
+        @contextlib.contextmanager
+        def _patched_find_module():


This is probably better in the global namespace than a local function because it clutters the code quite a bit. Sorry I wasn't clear about that before.

nicoddemus · 2017-12-12T09:50:01Z

@RonnyPfannschmidt I would love a second set of eyes on this.

RonnyPfannschmidt · 2017-12-12T10:05:11Z

i did a quick skim and it looks ok, im not fluent with the details either, i wonder if we should look into using importlib

cryvate · 2017-12-12T10:13:09Z

On python3.4+ we are basically using importlib: pkgutil.find_loader is a thin wrapper around importlib.util.find_spec there. We could use importlib.util.find_spec directly in python3 as I did in this commit, however this is largely equivalent to using pkgutil.find_loader when you check the source so I think at least until python2 support is dropped, it is better to stick with using pkgutil.find_loader.

coveralls · 2017-12-12T10:13:53Z

Coverage decreased (-0.2%) to 92.582% when pulling 1e29553 on cryvate:fix-issue-2985 into 5c6d773 on pytest-dev:master.

RonnyPfannschmidt · 2017-12-12T10:46:55Z

@cryvate thanks for elaborating 👍

nicoddemus

Thanks @cryvate!

@RonnyPfannschmidt we can merge IMO unless you want more time to study the PR.

RonnyPfannschmidt · 2017-12-13T12:57:07Z

i really dont have the time ^^

blueyed · 2018-10-11T11:48:06Z

testing/acceptance_test.py

+
+        monkeypatch.setenv('PYTHONPATH', join_pythonpath(*search_path))
+        for p in search_path:
+            monkeypatch.syspath_prepend(p)


@cryvate
Is the different order between PYTHONPATH and sys.path intentional here?
Fixed it in 2f058ad, which is required to fix the test failure when using realpath() on the args (18e7358), but have not really investigated why.

@blueyed
Though git blame will assign the 'blame' to me, actually this part of the code was a copy-paste from the test_cmdline_python_namespace_package test so I'd pass on the 'blame' to this PR. Regardless, I'd think that the different order (the opposite way around if I'm reading it correctly) is accidental here as well as in the other test.

@cryvate
Thanks and sorry for not looking good enough.

as well as in the other test

Which one do you mean?

@blueyed
Not a problem at all! I meant test_cmdline_python_namespace_package where I copy-pasted from.

…rgs.

cryvate force-pushed the fix-issue-2985 branch 2 times, most recently from 7ccc72d to 4e5344d Compare December 7, 2017 12:56

cryvate commented Dec 7, 2017

View reviewed changes

cryvate force-pushed the fix-issue-2985 branch from 4e5344d to df6885d Compare December 7, 2017 13:23

cryvate commented Dec 7, 2017

View reviewed changes

Fix issue 2985.

b62fd79

cryvate force-pushed the fix-issue-2985 branch from df6885d to b62fd79 Compare December 7, 2017 15:12

nicoddemus requested changes Dec 11, 2017

View reviewed changes

Improve testing comments and code of issue 2985 (symlink in pyargs).

dc19624

cryvate commented Dec 12, 2017

View reviewed changes

cryvate force-pushed the fix-issue-2985 branch 3 times, most recently from b462110 to 9703053 Compare December 12, 2017 08:35

Make patch for issue in pkgutil.ImpImporter local by using contextman…

3ca1e4b

…ager.

cryvate force-pushed the fix-issue-2985 branch from 9703053 to 3ca1e4b Compare December 12, 2017 08:46

nicoddemus reviewed Dec 12, 2017

View reviewed changes

Move _patched_find_module to module namespace.

1e29553

nicoddemus approved these changes Dec 13, 2017

View reviewed changes

RonnyPfannschmidt merged commit 476d4df into pytest-dev:master Dec 13, 2017

cryvate mentioned this pull request Dec 19, 2017

pyargs with subpackage breaks fixture (conftest.py?) discovery #2985

Closed

blueyed reviewed Oct 11, 2018

View reviewed changes

blueyed referenced this pull request Oct 11, 2018

Ensure that a module within a namespace package can be found by --pya…

e2e6e31

…rgs.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of pyargs #3010

Improve handling of pyargs #3010

cryvate commented Dec 7, 2017 •

edited

Loading

cryvate Dec 7, 2017

coveralls commented Dec 7, 2017

cryvate Dec 7, 2017

nicoddemus Dec 11, 2017

coveralls commented Dec 7, 2017

coveralls commented Dec 7, 2017

coveralls commented Dec 7, 2017

nicoddemus left a comment

nicoddemus Dec 11, 2017 •

edited

Loading

nicoddemus Dec 11, 2017

nicoddemus Dec 11, 2017

nicoddemus Dec 11, 2017

nicoddemus Dec 11, 2017

cryvate Dec 12, 2017

coveralls commented Dec 12, 2017

coveralls commented Dec 12, 2017

coveralls commented Dec 12, 2017

nicoddemus Dec 12, 2017

nicoddemus commented Dec 12, 2017

RonnyPfannschmidt commented Dec 12, 2017

cryvate commented Dec 12, 2017 •

edited

Loading

coveralls commented Dec 12, 2017

RonnyPfannschmidt commented Dec 12, 2017

nicoddemus left a comment

RonnyPfannschmidt commented Dec 13, 2017

blueyed Oct 11, 2018

cryvate Oct 11, 2018

blueyed Oct 11, 2018

cryvate Oct 11, 2018

Improve handling of pyargs #3010

Improve handling of pyargs #3010

Conversation

cryvate commented Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 7, 2017

coveralls commented Dec 7, 2017

coveralls commented Dec 7, 2017

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus Dec 11, 2017 • 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

Choose a reason for hiding this comment

coveralls commented Dec 12, 2017

coveralls commented Dec 12, 2017

coveralls commented Dec 12, 2017

Choose a reason for hiding this comment

nicoddemus commented Dec 12, 2017

RonnyPfannschmidt commented Dec 12, 2017

cryvate commented Dec 12, 2017 • edited Loading

coveralls commented Dec 12, 2017

RonnyPfannschmidt commented Dec 12, 2017

nicoddemus left a comment

Choose a reason for hiding this comment

RonnyPfannschmidt commented Dec 13, 2017

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

cryvate commented Dec 7, 2017 •

edited

Loading

nicoddemus Dec 11, 2017 •

edited

Loading

cryvate commented Dec 12, 2017 •

edited

Loading