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

Extract Patcher #519

Open
petr-motejlek opened this issue Feb 17, 2020 · 11 comments
Open

Extract Patcher #519

petr-motejlek opened this issue Feb 17, 2020 · 11 comments

Comments

@petr-motejlek
Copy link

Hey,

I’ve recently used two other libraries that allow one to stub out certain standard library functions, and I used them together with pyfakefs and really big kudos on how simple it is to patch things with pyfakefs in comparison ;)

Hence my thought: would you consider extracting the Patcher functionality into a separate package such that it would be possible to use it when patching other modules as well, as that would lead to less wheel reinvention?

I especially like how it tries to deal with all potential caveats, including allowing the user to have their SUT reloaded as a last resort. None of the other patchers I’ve seen so far do this!

@mrbean-bremen
Copy link
Member

Interesting idea, I haven't thought about this. Patcher currently has specific code for the file system functions, most of which can probably made configurable, and some of it (like creation of temp path and handling of open calls) might be handled by a derived class.

@jmcgeheeiv - what do you think?

@jmcgeheeiv
Copy link
Contributor

Well, @petr-motejlek, that's just about the highest compliment we could receive. Patcher is one of the major features added to pyfakefs after the Google era.

We are perhaps a bit blinkered by our focus on pyfakefs. Could you give us an example of how you would use an independent Patcher?

@mrbean-bremen
Copy link
Member

mrbean-bremen commented Feb 20, 2020

I have also thought about this a bit, and while I see the theoretical possibility to extract the patcher, I see no real use case at the moment. The Patcher alone is not sufficient - it also needs a fake implementation of the replaced module - at least this is the case in pyfakefs. Creating a fake module that simulates the real functionality (as opposed to a mock or stub) is not a trivial task.

But if there is any use real case I will be happy to go ahead and split out the Patcher. This can be done first in the same package, and if needed, it can be moved into a separate package later.

@petr-motejlek
Copy link
Author

Let's compare the way your internal patcher works to unittest.patch.

  • pyfakefs
    ** Has a fake implementation that it will replace the original with.
    ** Knows where the original it is replacing lives.
    ** Can look for a lot of ways how the original can be referenced in unit under test.
    ** Has an integrated reload functionality for when you need to reload the unit under test code (due to default arguments possibly storing a reference to the original).
    ** Is tied to pyfakefs.

  • unittest.patch
    ** Is not tied to any particular library (except for the standard python library).
    ** Has to be provided with a fake implementation to replace the original with.
    ** Has to be told specifically what to patch and it does not try to be smart at it, at all.

I think the extracted patcher should roughly work this way:
** I provide it with
*** the fake object it should use to replace the original with.
*** the path to the original.
*** optionally, the modules it should reload for me
*** if necessary, it could accept multiple combinations of (fake, original path, optional modules to reload) -- I think that would be a possible use case for pyfakefs

  • It should support being used as:
    ** decorator
    ** context manager

I think this way, you could even use it from within pyfakefs, or write pyfakefs in a way where I interface with both it and this patcher. Both are likely OK.

  • "The Patcher alone is not sufficient - it also needs a fake implementation of the replaced module - at least this is the case in pyfakefs."
    ** Well, for most cases using something like autospec_create from unittest is going to be enough. It's libraries like pyfakefs that might need to be more extensive, and you likely know best what configuration you would need with this extracted patcher to be able to make the magic happen.

Wen it comes to the usability of such a Patcher. I think sky would then be the limit. Again, going back to other faking libraries (such as responses or pytest-subprocess) They usually implement a Patcher themselves and sadly, yours is way smarter. I think if there was a library that did what your Patcher can do, they would use it. Which would make it less boilerplate to use other faking libraries.

Cheers

@mrbean-bremen
Copy link
Member

Thanks for that extensive explanation! You made your case, and "the sky is the limit" sounds good for a plan. 😀

I think I'm going to try to extract the patcher, and derive from it or configure it for pyfakefs, though this will take some time. If I succeed with that, @jmcgeheeiv can decide if he wants to split it into a separate package.

@mrbean-bremen
Copy link
Member

mrbean-bremen commented Feb 21, 2020

It probably makes sense to do this after the 4.0 release. I was actually thinking about making a release soon (we promised one for the beginningof the year), with Python 2 support removed and a few bug fixes already there.
@jmcgeheeiv, what do you think?

@petr-motejlek
Copy link
Author

Sounds great.

Kudos for deciding to drop Python 2. It's time the world migrated away from it :).

@mrbean-bremen
Copy link
Member

Getting back to this... I had played around with extracting Patcher inside pyfakefs, but didn't really get to a reasonable state. The problem is mostly that we have quite some special handling for the file system functionality, and factoring out the Patcher doesn't work very well.
I'm just thinking about the original proposal to make a separate package, that has most but not all of the functionality, specifically:

  • the ability to patch modules, classes, functions and default values (though probably using mock.patch)
  • the ability to patch dynamically loaded modules using DynamicPatcher
  • a decorator like patchfs, possibly with the patched module as argument, as a means to patch single tests, and maybe test classes
  • some of the custom arguments: modules_to_reload and skip_names (or skip_modules)

What I would omit (at least for the first version), are the unittest and pytest integration (instead providing examples on how to do this), and some of the more specific stuff.

I'm still not sure how useful that would be, but as have seen a lot of struggling to correctly use mock.patch or pytest-patch, it may make sense to do this.

@jmcgeheeiv - what do you think?

@jmcgeheeiv
Copy link
Contributor

jmcgeheeiv commented Oct 14, 2020

Perhaps enhance unittest.patch so that it satisfies @petr-motejlek's requirements?

@mrbean-bremen
Copy link
Member

Yes, something like that. A unittest.mock.patch-like interface (decorator and context manager) that tries to patch all instances of an object instead of only one like patch, probably very small at start, and then see if that gets somewhere.

@petr-motejlek
Copy link
Author

Yeah, that sounds good!

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

3 participants