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

PureWindowsPath.stem changes behavior when fs fixture is loaded #1006

Closed
kurtmckee opened this issue May 1, 2024 · 14 comments · Fixed by #1011
Closed

PureWindowsPath.stem changes behavior when fs fixture is loaded #1006

kurtmckee opened this issue May 1, 2024 · 14 comments · Fixed by #1011
Labels

Comments

@kurtmckee
Copy link
Contributor

Describe the bug

When the fs fixture is loaded, pathlib.PureWindowsPath.stem returns more than the file's stem.

How To Reproduce

mkdir demo
cd demo
python -m venv .venv
source .venv/bin/activate
pip install pytest pyfakefs

Create a file named test_demo.py with this content:

import pathlib

def test_basic():
    assert pathlib.PureWindowsPath(r"C:\example\demo.py").stem == "demo"

def test_pyfakefs(fs):
    assert pathlib.PureWindowsPath(r"C:\example\demo.py").stem == "demo"

Then run:

pytest test_demo.py

You will see the following output:

$ pytest test_demo.py 
============================== test session starts ===============================
platform linux -- Python 3.12.3, pytest-8.2.0, pluggy-1.5.0
rootdir: /home/kurt/dev/test
configfile: pyproject.toml
plugins: pyfakefs-5.4.1
collected 2 items                                                                

test_demo.py .F                                                            [100%]

==================================== FAILURES ====================================
_________________________________ test_pyfakefs __________________________________

fs = <pyfakefs.fake_filesystem.FakeFilesystem object at 0x7dec1ffc4e90>

    def test_pyfakefs(fs):
>       assert pathlib.PureWindowsPath(r"C:\example\demo.py").stem == "demo"
E       AssertionError: assert 'C:\\example\\demo' == 'demo'
E         
E         - demo
E         + C:\example\demo

/home/kurt/dev/test/test_demo.py:8: AssertionError
============================ short test summary info =============================
FAILED test_demo.py::test_pyfakefs - AssertionError: assert 'C:\\example\\demo' == 'demo'
========================== 1 failed, 1 passed in 0.08s ===========================

Your environment

Linux-6.5.0-28-generic-x86_64-with-glibc2.35
Python 3.12.3 (main, Apr 10 2024, 14:08:50) [GCC 11.4.0]
pyfakefs 5.4.1
pytest 8.2.0
@mrbean-bremen
Copy link
Member

Thank you! This is probably due to the handling of backslashes under Linux. Even if using PureWindowsPath, pyfakefs assumes Linux path separators, which is obviously not correct. This seems to be a rare use case, so it has not been encountered so far.

At the moment, you need to set the OS to Windows, if you want this to work, e.g.

import pathlib
from pyfakefs.fake_filesystem import OSType

def test_pyfakefs(fs):
    fs.os = OSType.WINDOWS
    assert pathlib.PureWindowsPath(r"C:\example\demo.py").stem == "demo"

@mrbean-bremen
Copy link
Member

I'm curious why you need this, anyway: do you want to test Windows behavior under Linux (in which case you would need to set the OS anyway), or is this some other use case?

@kurtmckee
Copy link
Contributor Author

kurtmckee commented May 1, 2024

I'm using PureWindowsPath when extracting and parsing shebang lines in arbitrary source code files because of its flexible handling of slashes and backslashes. Specifically for the Python launcher on Windows, shebang lines with absolute Windows paths and arguments are allowed, like this:

#!"C:\Program Files\Python39\python.exe" -v
print("Running on a specific interpreter, with arguments")

To accommodate this, I'm using PureWindowsPath to ensure that the path can be consistently parsed, regardless of the format of the shebang line, and regardless of the platform on which my tool is running. I don't think that I want to set fs.os to get the original parsing behavior back, as this may have other effects.

I'm very happy to submit a PR for this, and have already forked this repo for the purpose. If you can point me to the correct place to add/fix this, it'll give me a jump start (I'm assuming it's in fake_pathlib.py, but haven't dug into this beyond a cursory initial glance, which is why you're getting an initial bug report instead of a patch).

@mrbean-bremen
Copy link
Member

Thank you for the explanation - that makes sense!

A PR would be nice, of course, but I'm not sure if this is an easy fix. fake_pathlib.py indeed handles this. Here, we use as much of the original pathlib code as possible. Up to Python 3.11, there has been an adapter class that does the actual work, and which we replaced with a fake adapter class (which basically calls the fake os functions). In Python 3.12, this has gone, and the handling is done by using the os functions directly (which makes patching actually easier), but we have to support both, of course, as long as these version are around.

The changes probably need be done in PurePosixPath and PureWindowsPath, which are at the moment just wrappers around the real PurePath implementation.
Probably the handling needs to be adapted if the called class does not match the class set in the fake filesystem (e.g. instantiating a WindowsPath if the os is set to Linux in your case). The tricky part is that the separators and other fs-specific attributes that are used in the functions are set according to the fake OS. In the best case scenario, this just needs to be adapted in the initialization of PureWindowsPath and PurePosixPath, and the further handling can be done as usual. Or these attributes shall be used for all methods that are called by these classes, which may turn out a bit more complicated.

Anyway, any help is appreciated, and if you want to have a go at a PR, go ahead! But as I wrote, I'm not sure if this will be easy, so it is of course your decision if you want to try it.

@mrbean-bremen
Copy link
Member

mrbean-bremen commented May 2, 2024

@kurtmckee - do you want to have a go at this first, or shall I look at it myself?

@kurtmckee
Copy link
Contributor Author

Haha, "but I've been working to get the test suite just right to prepare to work on this..."! 😆

If you already know what the change will look like, please go for it! Otherwise, I'll leave a note on this issue when I have availability to dig in again. 👍

@mrbean-bremen
Copy link
Member

Haha, "but I've been working to get the test suite just right to prepare to work on this..."! 😆

Well, I don't want to deprive you of that work 😁

I don't know how the change will look like yet, though I may have a closer look at the weekend. If I find something out, I'll let you know. If you get the time to work on it, I would prefer that (especially after seeing how fast you resolved the test issue). I like it that somebody else has a look at the code, to get another perspective - I mostly work alone on this project, which I find a bit unfortunate.

@kurtmckee
Copy link
Contributor Author

I like it that somebody else has a look at the code

That makes total sense to me! I'll tackle this one. 😁

kurtmckee added a commit to kurtmckee/pr-pyfakefs that referenced this issue May 4, 2024
kurtmckee added a commit to kurtmckee/pr-pyfakefs that referenced this issue May 4, 2024
kurtmckee added a commit to kurtmckee/pr-pyfakefs that referenced this issue May 4, 2024
@kurtmckee
Copy link
Contributor Author

I ended up having a flight delayed for several hours, which gave me an opportunity to look into this. 👍

mrbean-bremen pushed a commit that referenced this issue May 6, 2024
* Hard-code path separators in `Pure*Path` classes
* Fixes #1006
@mrbean-bremen
Copy link
Member

Thank you! Do you need a patch release, or can you wait a bit?

@kurtmckee
Copy link
Contributor Author

This will unblock some work I was doing, but I won't be able to return to that work until next week at the earliest. Please don't feel pressure to get this out the door ASAP!

Thanks for your work on pyfakefs!

@mrbean-bremen
Copy link
Member

Ok, I will check if I can handle another issue first, and make a patch release after that.

@mrbean-bremen
Copy link
Member

FYI: The release is out.

@kurtmckee
Copy link
Contributor Author

Awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants