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

[WIP] Make fake filesystem, fake os and fake os.path respect additional_skip_names #1025

Merged
merged 7 commits into from
Aug 15, 2024

Conversation

sassanh
Copy link
Contributor

@sassanh sassanh commented May 27, 2024

Describe the changes

Add a call stack check for fake filesystem, os and os.path methods in their handle_original_call wrapper to see if they are being called from a module listed in skip_modules.

Tasks

  • Unit tests added that reproduce the issue or prove feature is working
  • Fix or feature added
  • Entry to release notes added
  • Pre-commit CI shows no errors
  • Unit tests passing
  • For documentation changes: The Read the Docs preview builds and looks as expected

@sassanh
Copy link
Contributor Author

sassanh commented May 27, 2024

This is mostly brainstorm material to see if we want to go in this direction.

Implementation details:
Most of the job is done as a filter in wrapper functions, is_called_from_skipped_module starts from the end of the call stack and goes up until it reaches a module that is:

  1. Is not a frozen importlib
  2. outside of Python's stdlib
  3. outside of pyfakefs library itself (except for tests/ and pytest_tests/ directories)
    This module is considered the module that is calling the wrapped function and if it is listed in skip_names, then it will try to run the original function instead of the fake wrapped function.

It is working for Python 3.12, 3.11 and mostly 3.10, but 3.9, 3.8 and 3.7 have yet different internals for pathlib.

@sassanh
Copy link
Contributor Author

sassanh commented Jun 10, 2024

@mrbean-bremen
When you got the time, please check it and let me know if you approve this approach.
If it looks good to you, I will work on tests for other Python versions and Windows.

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - as noted, I like that approach. There will be some work to do for the other Python versions (I don't think there is special handling needed for Windows), and we have to check the performance after the changes, but I think this is on a good way! 🚀

pyfakefs/helpers.py Show resolved Hide resolved
pyfakefs/fake_os.py Show resolved Hide resolved
@mrbean-bremen
Copy link
Member

@sassanh - are you still working on this, or have you given up?

@sassanh
Copy link
Contributor Author

sassanh commented Jul 31, 2024

I'm not actively working on it but I have it on my radar.
If the open pull request is causing trouble in organization we can close it.
The blocking issue is still updating the tests for older python versions. The thing is I don't know how much work is needed to address these issues.

@mrbean-bremen
Copy link
Member

The PR isn't blocking anything - I just wanted to know if you are still on it, or if you need help. Take all the time you need - and thanks for the effort!

@sassanh
Copy link
Contributor Author

sassanh commented Aug 1, 2024

Feel free to add any commits or make any changes to this pull request as needed. Your help is greatly appreciated! I might not have the capacity to address tests for older versions in the new future.

@mrbean-bremen mrbean-bremen force-pushed the skip_names branch 3 times, most recently from 4b6aecd to e02c3dc Compare August 12, 2024 18:42
- fix wrapped functions in pathlib (accessor not available in most Python versions)
- fix open_code handling
- make module path comparisons case-insensitive under Windows and macOS
- disable one failing test for PyPy 3.7
@mrbean-bremen
Copy link
Member

@sassanh - I've rebased against main and fixed the failing tests (disabled one test for PyPy 3.7, as I didn't want to invest time in that outdated version), but other than that, only the release notes are missing as far as I can see.
Please check my commit and add the release notes (and anything you still want to add), and we can go and merge this.

@sassanh
Copy link
Contributor Author

sassanh commented Aug 13, 2024

@mrbean-bremen Thank you so much! I appreciate your help!
I will check your commit and add the release notes in the weekend.

@sassanh
Copy link
Contributor Author

sassanh commented Aug 15, 2024

@mrbean-bremen I checked the changes and it all looks good to me. I added the release notes too. Let's merge it if it looks good to you.

CHANGES.md Outdated Show resolved Hide resolved
Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that! This change was something that I had in mind for quite some time, but never really got to it.

updates:
- [github.com/asottile/pyupgrade: v3.16.0 → v3.17.0](asottile/pyupgrade@v3.16.0...v3.17.0)
- [github.com/astral-sh/ruff-pre-commit: v0.5.4 → v0.5.5](astral-sh/ruff-pre-commit@v0.5.4...v0.5.5)
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.5.5 → v0.5.6](astral-sh/ruff-pre-commit@v0.5.5...v0.5.6)
- [github.com/pre-commit/mirrors-mypy: v1.11.0 → v1.11.1](pre-commit/mirrors-mypy@v1.11.0...v1.11.1)
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.5.6 → v0.5.7](astral-sh/ruff-pre-commit@v0.5.6...v0.5.7)
@mrbean-bremen mrbean-bremen merged commit c954966 into pytest-dev:main Aug 15, 2024
67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants