classification
Title: mock patch should match behavior of import from when module isn't present in sys.modules
Type: behavior Stage: patch review
Components: Tests Versions: Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: dino.viehland Nosy List: cjw296, dino.viehland, xtreak
Priority: normal Keywords: patch

Created on 2020-02-04 20:27 by dino.viehland, last changed 2020-02-10 19:28 by dino.viehland.

Pull Requests
URL Status Linked Edit
PR 18347 open dino.viehland, 2020-02-04 20:36
Messages (9)
msg361366 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2020-02-04 20:27
The fix for bpo-17636 added support for falling back to sys.modules when a module isn't directly present on the module.  But mock doesn't have the same behavior - it'll try the import, and then try to get the value off the object.  If it's not there it just errors out.

Instead it should also consult sys.modules to be consistent with import semantics.
msg361368 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2020-02-04 21:16
What's the real world use case for this?
msg361370 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2020-02-04 21:31
It's related to bpo-39336.  If you have an immutable package which doesn't allow it's children to be published on it when following the chain of packages it ends up not arriving at the final module.

But you can also hit it if you end up doing the patch during a relative import, although that seems much less likely.  But it generally seems matching the behavior of imports would be good to me.
msg361376 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2020-02-04 22:12
I'm afraid I don't understand "immutable package which doesn't allow it's children to be published on it", can you give an example?
msg361377 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2020-02-04 23:10
My actual scenario involves a custom module loader where the modules are published are completely immutable (it ends up publishing an object which isn't a subtype of module).  It can still have normal Python modules as a child which aren't immutable, so they could still be patched by Mock (or it could have immutable sub-packages which Mock wouldn't be able to patch).  

So imagine something like this:

immutable_package\__init__.py
__immutable__ = True
x = 2

immutable_package\x.py
y = 2


Doing a "from immutable_package import x" would normally publish "x" as a child onto the package.  But because the package is immutable, this is impossible, and the assignment is ignored with a warning.  

When Mock gets a call to patch on something like "immutable_package.x.y", it's not going to find x, even though if I were to write "from immutable_package.x import y" or "from immutable_package import x" it would succeed.

Cases can be contrived without all of this though where the child isn't published on it's parent, but it requires 

x/__init__.py
from x.pkg import child

x/pkg/__init__.py:
x = 1


x/pkg/child.py:
from unittest.mock import patch
y = 42

@patch('x.pkg.child.y', 100)
def f():
    print(y)

f()

"python -m x" will fail without the patch but succeed with it.
msg361501 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2020-02-06 18:55
Apologies, but I'm still not sure what "the modules are published" means?
"publish "x" as a child onto the package" also doesn't mean much to me, I'm afraid. Are you aware of any importlib docs or some such which might be able to explain this to me?

When "It can still have normal Python modules as a child which aren't immutable", what happens when you try to patch the immutable module? What happens if you try to patch the mutable module below it?

When "the assignment is ignored with a warning.", what is doing that ignoring?

Unless I'm missing something, this feels like such an edge case I'm not sure mock.patch should be trying to support it. Should this be something that is handled by your immutable import thing?
msg361504 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2020-02-06 21:27
Sorry, publish may not necessarily be the best term.  When you do "from foo import bar" and foo is a package Python will set the bar module onto the foo module.  That leads to the common mistake of doing "import foo" and then "foo.bar.baz" and later removing the thing that do "from foo import bar" and having things blow up later.

Without additional support there's no way to patch the immutable module.  We can provide a mode where we enable the patching for testing and disable it in a production environment though.  That basically just involves passing a proxy object down to Mock.  And monkey patching the mutable module is perfectly fine.

The thing that's doing the ignoring of the assignment is the import system.  So it's now okay if the package raises an AttributeError.

There's not really a great way to work around this other than just bypassing mock's resolution of the object here - i.e. replacing mock.patch along with _get_target, _importer, and _dot_lookup and calling mock._patch directly, which isn't very nice (but is do-able).

And while this is a strange way to arrive at a module existing in sys.modules but not being on the package it is something that can happen in the normal course of imports, hence the reason why the import system handles this by checking sys.modules today.  It's also just turning what is currently an error while mocking into a success case with a simple few line change.
msg361661 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2020-02-09 20:12
Hmm, the more we get into this, the less comfortable I am of the patch in the PR.

Instead of copying and pasting more code that's likely to get out of sync, could you change the PR to just replace _importer with the correct parts of importlib to support your usecase?

Keep the unit test though, useful to show in mock why the change is needed, and make sure we do something sensible in the backport!
msg361709 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2020-02-10 19:28
I like that idea, let me see if I can find a way to do that.  This is a little bit different in that it's implicitly trying to find a module, and supports dotting through non-packages as well, but maybe there's a way to leverage importlib and still keep the existing behavior.
History
Date User Action Args
2020-02-10 19:28:27dino.viehlandsetmessages: + msg361709
2020-02-09 20:12:34cjw296setmessages: + msg361661
2020-02-06 21:27:50dino.viehlandsetmessages: + msg361504
stage: needs patch -> patch review
2020-02-06 18:55:01cjw296setmessages: + msg361501
2020-02-04 23:10:23dino.viehlandsetmessages: + msg361377
2020-02-04 22:12:03cjw296setmessages: + msg361376
2020-02-04 21:49:06xtreaksetnosy: + xtreak
2020-02-04 21:31:15dino.viehlandsetmessages: + msg361370
stage: patch review -> needs patch
2020-02-04 21:16:29cjw296setnosy: + cjw296
messages: + msg361368
2020-02-04 20:36:51dino.viehlandsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request17721
2020-02-04 20:27:49dino.viehlandcreate