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

Plans to support io.open_code? #554

Closed
berleant opened this issue Oct 1, 2020 · 11 comments
Closed

Plans to support io.open_code? #554

berleant opened this issue Oct 1, 2020 · 11 comments

Comments

@berleant
Copy link

berleant commented Oct 1, 2020

Is your feature request related to a problem? Please describe.

In python 3.8.1+ my tests are failing because of this change: python/cpython#17244

This appears to be because pyfakefs' io only mocks io.open (http://jmcgeheeiv.github.io/pyfakefs/release/modules.html?highlight=io#pyfakefs.fake_filesystem.FakeIoModule)

This is somewhat disappointing as pyfakefs has worked perfectly until now.

Describe the solution you'd like

pyfakefs supports io.open_code. Full disclosure: I have no idea how feasible this is. The implementation appears to be in C (https://github.com/python/cpython/blob/47a23fc63fa5df2da8dbc542e78e521d4a7f10c9/Modules/_io/_iomodule.c#L520) but there is this also this pure python implementation that uses open(), which apparently exists "for tests": https://github.com/python/cpython/blob/942f7a2dea2e95a0fa848329565c0d0288d92e47/Lib/_pyio.py#L259

Describe alternatives you've considered
For now, I'm only testing with 3.8.0. For a more permanent solution, I could mock it by hand using the mock module. The easiest thing, however, would be to rework the tests so that they use the actual file system.

@mrbean-bremen
Copy link
Member

Thanks for the report! I hadn't seen open_code before (missed that in the changes for Python 3.8).
The easiest way to fix this would be to do just the same as the Python implementation which just uses open, but that would not implement the full functionality. It does not look to me like that functionality can be added in pure Python, but I have to take a closer look at this.
Did I understand you correctly, that you do not use this function explicitely, but it is called by some other function internally? Would it be sufficient for you to just use the open functionality in pyfakefs for the time being?

@berleant
Copy link
Author

berleant commented Oct 2, 2020

Did I understand you correctly, that you do not use this function explicitely, but it is called by some other function internally?

Yes, that's correct--I am using runpy.run_path, which calls io.open_code starting in version 3.8.1.

Thanks for the quick response!

@mrbean-bremen
Copy link
Member

mrbean-bremen commented Oct 3, 2020

Hm, that turns out to be a bit more complicated than I thought. If I fake open_code, a few tests (specifically tests for pandas) fail because open_code is called internally by some modules to, well, open code files. As these (e.g. the compiled Python files) are not present in the fake file system, these tests fail. So, faking open_code unconditionally potentially breaks a lot of tests (while not faking it breaks others).
Maybe it makes sense to add an option for this.
I would also be interested in a minimal reproducible example for the failing test to understand why in this case open_code has to be patched (as opposed to the cases where it reads pyc files of the packages themselves). Maybe we could add some heuristics as to when to patch it and when not.

mrbean-bremen added a commit to mrbean-bremen/pyfakefs that referenced this issue Oct 4, 2020
- added new argument 'patch_open_code' to allow patching the function
- see pytest-dev#554
mrbean-bremen added a commit to mrbean-bremen/pyfakefs that referenced this issue Oct 4, 2020
- added new argument 'patch_open_code' to allow patching the function
- see pytest-dev#554
@mrbean-bremen
Copy link
Member

@shoshber - I added a new argument patch_open_code that by default is set to False. You can set it in setUpPyfakefs, if you are using unittest. In pytest, it's a bit awkward to configure--you have to use @pytest.mark.parametrize('fs', [[None, None, None, True, True, True]], indirect=True)--probably better use a customized fixture (an example is shown here.
Please check if that works for you in master. I'm also still interested in some example test that shows the problem - maybe we can find a better solution.

@berleant
Copy link
Author

berleant commented Oct 5, 2020

faking open_code unconditionally potentially breaks a lot of tests (while not faking it breaks others).

Indeed--in fact it breaks mine 🤣 I should have seen that coming--the code under test uses runpy.run_module in addition to runpy.run_path. The fix in master does exactly what I asked though, so thank you (especially for the quick turnaround!). Here's a minimal reproducible example, as requested:
example.zip (after unzipping, run python -m unittest example.test)

@mrbean-bremen
Copy link
Member

I'm confused--your example works for me without patching, but fails if open_code is patched. So you have another example that uses runpy.run_path and the behavior is the opposite one? Or are both used in the same test, and the patch_open_code option doesn't help you therefore?

@berleant
Copy link
Author

berleant commented Oct 6, 2020

Color me confused as well. I think this example explains it better: example.zip. Here is how I run the example:

$ git clone https://github.com/jmcgeheeiv/pyfakefs
$ unzip example.zip
$ virtualenv -p python3.8 venv
$ source venv/bin/activate
$ pip install -e pyfakefs
$ python -m unittest example.test
(2 out of 3 tests fail)

Or are both used in the same test, and the patch_open_code option doesn't help you therefore?

If I understand what you're saying, that describes my situation most closely.

Again, thank you for your help! If this isn't feasible, I understand and can find an alternative.

@mrbean-bremen
Copy link
Member

Thanks - didn't get to it today, but I will have a look tomorrow. Maybe I'll get an idea...

@mrbean-bremen
Copy link
Member

Ok, I changed the argument to now take an enum (fake_filesystem.PatchMode) which can be set to OFF (default), ON and AUTO. OFF and ON behave like False and True before, but in AUTO mode open_code is only mocked, if the file it shall open exists in the fake filesystem. That works at least for your minimal example (I actually added it as a test) - please check if that works for you in the real code.

@berleant
Copy link
Author

It works beautifully, thank you! This might be the most positive and pleasant experience I've ever had on an issue thread.

@mrbean-bremen
Copy link
Member

Thanks 😄 -- always good to know we could help someone! We will probably make a new release soon, if nothing else comes up.
Closing this issue.

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

No branches or pull requests

2 participants