Fix assertion rewriter crash if cwd changes mid-testing #3980

nicoddemus · 2018-09-13T22:11:22Z

Unfortunately we need to get a py.path.local object to perform the fnmatch
operation, it is different from the standard fnmatch module because it
implements its own custom logic. So we need to use py.path to perform
the fnmatch for backward compatibility reasons.

Ideally we should be able to use a "pure path" in pathlib terms (a path
not bound to the file system), but we don't have those in pylib.

Fix #3973

nicoddemus · 2018-09-13T22:11:43Z

src/_pytest/assertion/rewrite.py

-        fn_pypath = py.path.local(os.path.sep.join(parts))
+        try:
+            fn_pypath = py.path.local(os.path.sep.join(parts))
+        except EnvironmentError:


I'm not happy with that approach, and I'm open to suggestions.

lets use a pathlib path instance or a string instead, i - anything thats not turning itself into a absolute path by force will be good

Problem is the fnmatch call we need to perform a little further down in the code, see my description on the PR.

https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.match

Didn't know about that function TBH, but the main point is that py.path.local.fmatch has its own implementation, which we know from past experience (#2140) that it diverges from the standard fnmatch module and from pathlib2.PurePath.match.

If we use different fnmatch algorithms, I'm afraid we will start to see false positives here (like happened in #2140) and not rewrite some modules.

i reviewed pathlib.Path.match - it does it completely correct and better than localpath - as such i consider a port to pathlib required

coveralls · 2018-09-13T22:49:27Z

Coverage increased (+0.02%) to 93.835% when pulling 7f48f55 on nicoddemus:rewrite-cwd-changed into bb57186 on pytest-dev:master.

RonnyPfannschmidt · 2018-09-15T05:24:44Z

Let's extract the smarter than thou fnmatcher then

RonnyPfannschmidt

lets use pathlib with purepaths here

nicoddemus · 2018-09-16T15:43:52Z

Thanks for the leg work.

I wrote a test to check the compatibility, and they do match the same paths a lot, with a few exceptions:

import os
import sys

import py

import pytest
from _pytest.compat import PurePath


class Test:

    @pytest.fixture(params=['py.path', 'pathlib'])
    def match(self, request):
        if request.param == 'py.path':
            def match_(pattern, path):
                return py.path.local(path).fnmatch(pattern)
        else:
            assert request.param == 'pathlib'
            
            def match_(pattern, path):
                return PurePath(path).match(pattern)
        return match_

    if sys.platform == 'win32':
        drv1 = 'c:'
        drv2 = 'd:'
    else:
        drv1 = ''
        drv2 = ''

    @pytest.mark.parametrize('pattern, path', [
        ('*.py', 'foo.py'),
        ('*.py', 'foo/foo.py'),
        ('tests/*.py', 'tests/foo.py'),
        (drv1 + '/*.py', drv1 + '/foo.py'),
        (drv1 + '/foo/*.py', drv1 + '/foo/foo.py'),
        ('tests/**/test*.py', 'tests/test_foo.py'),
        ('tests/**/test*.py', 'tests/foo/test_foo.py'),
        ('tests/**/doc/test*.py', 'tests/foo/bar/doc/test_foo.py'),
        ('tests/**/doc/**/test*.py', 'tests/foo/doc/bar/test_foo.py'),
    ])
    @pytest.mark.parametrize('normalize', [ False])
    def test_matching(self, match, pattern, path, normalize):
        assert match(pattern, path if not normalize else os.path.normpath(path))

    @pytest.mark.parametrize('pattern, path', [
        ('*.py', 'foo.pyc'),
        ('*.py', 'foo/foo.pyc'),
        ('tests/*.py', 'foo/foo.py'),
        (drv1 + '/*.py', drv2 + '/foo.py'),
        (drv1 + '/foo/*.py', drv2 + '/foo/foo.py'),
        ('tests/**/test*.py', 'tests/foo.py'),
        ('tests/**/test*.py', 'foo/test_foo.py'),
        ('tests/**/doc/test*.py', 'tests/foo/bar/doc/foo.py'),
        ('tests/**/doc/test*.py', 'tests/foo/bar/test_foo.py'),
    ])
    @pytest.mark.parametrize('normalize', [True, False])
    def test_not_matching(self, match, pattern, path, normalize):
        assert not match(pattern, path if not normalize else os.path.normpath(path))

Here are the failures on my system:

FAIL testing/test_path_match_compatibility.py::Test::()::test_matching[py.path-tests/**/test*.py-tests/test_foo.py]
FAIL testing/test_path_match_compatibility.py::Test::()::test_matching[pathlib-tests/**/test*.py-tests/test_foo.py]
FAIL testing/test_path_match_compatibility.py::Test::()::test_matching[pathlib-tests/**/doc/test*.py-tests/foo/bar/doc/test_foo.py]```

What do you take from those divergences?

RonnyPfannschmidt · 2018-09-16T18:09:50Z

thanks for the detail analysis test automation - i believe this is caused by path libs segment splitting, which sorts out different pathological cases than pytest does

nicoddemus · 2018-09-16T18:20:26Z

Yep that was my analysis as well. How do you like us to move forward? I can port the FNMatcher code, but now we will have to keep it around "forever". Another alternative would be for us to bite the bullet and move this to the features branch as a "breaking change". Breaking change in quotes because I believe very few people will be affected by this, as is a quite corner case.

What are your thoughts?

RonnyPfannschmidt · 2018-09-16T18:41:25Z

lets go for path.local right now ,record the finding and plan it as a breaking change - since it is one (stuff like this is how setuptools is >40 today

nicoddemus · 2018-09-16T18:55:41Z

lets go for path.local right now

Just to confirm, you mean porting FNMatcher correct?

RonnyPfannschmidt · 2018-09-16T19:44:30Z

@nicoddemus no - i mean still using a py.path.local as is right now and preparing for the breaking release later

there is zero value in porting fnmatcher now if we will kill it later - we dont have time for that kind of high cost throw-away code

nicoddemus · 2018-09-16T22:08:17Z

Oh OK. Anything else you would like to do here then?

RonnyPfannschmidt

No, sorry for forgetting the approval

nicoddemus · 2018-09-17T12:26:27Z

@RonnyPfannschmidt I was thinking about this a little more in the shower today: the FNMatcher implementation is really simple, and it does allow for matches that pathlib does not: tests/**/doc/test*.py. I think we should reconsider porting it and keep it as part of pytest, it is easy to test and should be 6-10 lines of code (plus tests). What do you say?

RonnyPfannschmidt · 2018-09-17T13:24:04Z

@nicoddemus in that case i'll agree in which case i'd also like to see a bug report against pathlib accompanying it

Unfortunately we need to get a `py.path.local` object to perform the fnmatch operation, it is different from the standard `fnmatch` module because it implements its own custom logic. So we need to use `py.path` to perform the fnmatch for backward compatibility reasons. Ideally we should be able to use a "pure path" in `pathlib` terms (a path not bound to the file system), but we don't have those in pylib. Fix pytest-dev#3973

…rewrite This way we don't need to have real file system path, which prevents the original pytest-dev#3973 bug.

nicoddemus · 2018-09-18T21:15:58Z

@RonnyPfannschmidt ready for review (ignore the CI failure as that's due to codecov)

RonnyPfannschmidt

this is fabulously done, i'd like to report a bug against python still to refer it here (i consider it a oversight in pathlib)

RonnyPfannschmidt · 2018-09-19T05:35:45Z

https://bugs.python.org/issue34731 tracks this for python - lets add a reference, then its ready to merge

RonnyPfannschmidt · 2018-09-19T08:16:49Z

https://bugs.python.org/issue29249 is an older issue thats the relevant one

nicoddemus · 2018-09-19T11:21:20Z

lets add a reference, then its ready to merge

Thanks for searching the relevant issues, done: 1e2e65f

codecov · 2018-09-19T11:21:48Z

Codecov Report

Merging #3980 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3980      +/-   ##
==========================================
+ Coverage   94.48%   94.53%   +0.05%     
==========================================
  Files         107      108       +1     
  Lines       23663    23703      +40     
  Branches     2349     2353       +4     
==========================================
+ Hits        22358    22408      +50     
+ Misses        994      988       -6     
+ Partials      311      307       -4

Flag	Coverage Δ
#doctesting	`29.31% <42.3%> (+0.05%)`	⬆️
#linux	`94.38% <90.38%> (+0.03%)`	⬆️
#nobyte	`0% <0%> (ø)`	⬆️
#numpy	`28.23% <42.3%> (+0.05%)`	⬆️
#pexpect	`0% <0%> (ø)`	⬆️
#py27	`92.64% <94.23%> (+0.05%)`	⬆️
#py34	`92.13% <92.3%> (+0.04%)`	⬆️
#py35	`92.14% <92.3%> (+0.04%)`	⬆️
#py36	`92.71% <92.3%> (+0.05%)`	⬆️
#py37	`92.34% <92.3%> (+0.04%)`	⬆️
#trial	`31.27% <42.3%> (+0.05%)`	⬆️
#windows	`93.83% <88.46%> (-0.02%)`	⬇️
#xdist	`18.5% <40.38%> (+0.03%)`	⬆️

Impacted Files	Coverage Δ
src/_pytest/assertion/rewrite.py	`95.67% <100%> (-0.03%)`	⬇️
testing/test_assertrewrite.py	`83.78% <100%> (+0.1%)`	⬆️
src/_pytest/paths.py	`100% <100%> (ø)`	⬆️
testing/test_paths.py	`100% <100%> (ø)`
src/_pytest/compat.py	`96.62% <100%> (+0.07%)`	⬆️
src/_pytest/terminal.py	`91.6% <0%> (+1.74%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb57186...7f48f55. Read the comment docs.

nicoddemus requested a review from RonnyPfannschmidt September 13, 2018 22:11

nicoddemus commented Sep 13, 2018

View reviewed changes

RonnyPfannschmidt requested changes Sep 16, 2018

View reviewed changes

RonnyPfannschmidt approved these changes Sep 17, 2018

View reviewed changes

nicoddemus force-pushed the rewrite-cwd-changed branch from 2a5ad99 to f642728 Compare September 17, 2018 23:05

Use a PurePath instance to do matching against patterns in assertion …

37d2469

…rewrite This way we don't need to have real file system path, which prevents the original pytest-dev#3973 bug.

nicoddemus force-pushed the rewrite-cwd-changed branch from f642728 to 37d2469 Compare September 17, 2018 23:29

RonnyPfannschmidt approved these changes Sep 19, 2018

View reviewed changes

Add references to the relevant Python issues

1e2e65f

Fix linting

7f48f55

RonnyPfannschmidt merged commit 29dac03 into pytest-dev:master Sep 19, 2018

Uh oh!

Fix assertion rewriter crash if cwd changes mid-testing #3980

Fix assertion rewriter crash if cwd changes mid-testing #3980

Uh oh!

Conversation

nicoddemus commented Sep 13, 2018

Uh oh!

nicoddemus Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

RonnyPfannschmidt Sep 14, 2018

Choose a reason for hiding this comment

Uh oh!

nicoddemus Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonnyPfannschmidt Sep 14, 2018

Choose a reason for hiding this comment

Uh oh!

nicoddemus Sep 14, 2018

Choose a reason for hiding this comment

Uh oh!

RonnyPfannschmidt Sep 16, 2018

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RonnyPfannschmidt commented Sep 15, 2018

Uh oh!

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

nicoddemus commented Sep 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RonnyPfannschmidt commented Sep 16, 2018

Uh oh!

nicoddemus commented Sep 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RonnyPfannschmidt commented Sep 16, 2018

Uh oh!

nicoddemus commented Sep 16, 2018

Uh oh!

RonnyPfannschmidt commented Sep 16, 2018

Uh oh!

nicoddemus commented Sep 16, 2018

Uh oh!

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

nicoddemus commented Sep 17, 2018

Uh oh!

RonnyPfannschmidt commented Sep 17, 2018

Uh oh!

nicoddemus commented Sep 18, 2018

Uh oh!

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

RonnyPfannschmidt commented Sep 19, 2018

Uh oh!

RonnyPfannschmidt commented Sep 19, 2018

Uh oh!

nicoddemus commented Sep 19, 2018

Uh oh!

codecov bot commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nicoddemus Sep 14, 2018 •

edited

Loading

coveralls commented Sep 13, 2018 •

edited

Loading

nicoddemus commented Sep 16, 2018 •

edited

Loading

nicoddemus commented Sep 16, 2018 •

edited

Loading

codecov bot commented Sep 19, 2018 •

edited

Loading