Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

path.common: handle FileNotFoundError when trying to import pathlib #207

Merged
merged 3 commits into from Apr 5, 2019
Merged

path.common: handle FileNotFoundError when trying to import pathlib #207

merged 3 commits into from Apr 5, 2019

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Feb 13, 2019

Python 3.4 might raise FileNotFoundError due to os.getcwd() failing on
a non-existing cwd. This is fixed in Python 3.5.

Ref: pytest-dev/pytest#4787 (comment)

@blueyed blueyed mentioned this pull request Feb 13, 2019
Merged
@blueyed
Copy link
Contributor Author

blueyed commented Feb 13, 2019

Code in py34: https://github.com/python/cpython/blob/fa7193286099f8e4164f4a795afd5ad4a8e229df/Lib/importlib/_bootstrap.py#L1878-L1879

@RonnyPfannschmidt
Copy link
Member

did this practically happen?

@blueyed
Copy link
Contributor Author

blueyed commented Feb 13, 2019

Yes, with py34-xdist on Travis, see the original ref.

@blueyed
Copy link
Contributor Author

blueyed commented Feb 13, 2019

@RonnyPfannschmidt
https://github.com/pytest-dev/pytest/pull/4787/files#diff-606d01f030119020f3e820547882b6b0

@RonnyPfannschmidt
Copy link
Member

Yikes, thanks

@blueyed
Copy link
Contributor Author

blueyed commented Feb 14, 2019

Rebased.

@blueyed
Copy link
Contributor Author

blueyed commented Feb 14, 2019

Pushed a fixup.

I also wonder if caching the ImportError wouldn't make sense?!

@nicoddemus
Copy link
Member

Can you please check the travis failure @blueyed?

@blueyed
Copy link
Contributor Author

blueyed commented Feb 18, 2019

I think it is related to pytest itself failing with the test.
Will have to investigate.

@blueyed
Copy link
Contributor Author

blueyed commented Feb 18, 2019

IIRC AppVeyor skips the test.

nicoddemus
nicoddemus reviewed Feb 19, 2019
View reviewed changes
testing/path/test_local.py Outdated Show resolved Hide resolved
testing/path/test_local.py Show resolved Hide resolved
@nicoddemus nicoddemus mentioned this pull request Feb 19, 2019
Merged
@nicoddemus
Copy link
Member

Any clues about the failures on Travis?

@blueyed
Copy link
Contributor Author

blueyed commented Feb 22, 2019

Still the same as with #207 (comment) I guess.
No energy to look into this now.

@nicoddemus
Copy link
Member

OK. Another option is to drop support for those old pytest versions.

With the "importlib" option getting used in pytest, we will need to require a higher version of py anyway.

@blueyed
path.common: handle FileNotFoundError when trying to import pathlib
33afe63 
Python 3.4 might raise FileNotFoundError due to `os.getcwd()` failing on
a non-existing cwd.  This is fixed in Python 3.5.

Ref: pytest-dev/pytest#4787 (comment)
@blueyed
Handle missing FileNotFoundError
a64cb3f 
Might not really be necessary, since py27 triggers the ImportError
always already, but better to be safe.
@blueyed
Copy link
Contributor Author

blueyed commented Apr 5, 2019

Fixed the new test to cd back (using monkeypatch).

@codecov-io
Copy link

Codecov Report

Merging #207 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
+ Coverage   82.04%   82.07%   +0.03%     
==========================================
  Files          55       55              
  Lines       10136    10153      +17     
  Branches     1141     1141              
==========================================
+ Hits         8316     8333      +17     
  Misses       1556     1556              
  Partials      264      264

@nicoddemus
Copy link
Member

Please add a CHANGELOG then it is good to go I think

@blueyed
Copy link
Contributor Author

blueyed commented Apr 5, 2019

Pushed with [ci skip] - hopefully that will still trigger it on the merge.

@nicoddemus nicoddemus merged commit 2802b18 into pytest-dev:master Apr 5, 2019
@blueyed blueyed deleted the py34-fspath-filenotfound branch April 8, 2019 03:03
@blueyed blueyed mentioned this pull request Apr 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicoddemus nicoddemus nicoddemus left review comments

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@blueyed @RonnyPfannschmidt @nicoddemus @codecov-io