pytest-dev / py

Python development support library (note: maintenance only)

MIT License

67 stars 106 forks source link

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

Closed blueyed closed 5 years ago

blueyed commented 5 years ago

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

Ref: https://github.com/pytest-dev/pytest/pull/4787#issuecomment-463341251

blueyed commented 5 years ago

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

RonnyPfannschmidt commented 5 years ago

did this practically happen?

blueyed commented 5 years ago

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

blueyed commented 5 years ago

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

RonnyPfannschmidt commented 5 years ago

Yikes, thanks

blueyed commented 5 years ago

Rebased.

blueyed commented 5 years ago

Pushed a fixup.

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

nicoddemus commented 5 years ago

Can you please check the travis failure @blueyed?

blueyed commented 5 years ago

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

blueyed commented 5 years ago

IIRC AppVeyor skips the test.

nicoddemus commented 5 years ago

Any clues about the failures on Travis?

blueyed commented 5 years ago

Still the same as with https://github.com/pytest-dev/py/pull/207#issuecomment-464888877 I guess. No energy to look into this now.

nicoddemus commented 5 years ago

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 commented 5 years ago

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

codecov-io commented 5 years ago

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 commented 5 years ago

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

blueyed commented 5 years ago

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