isinstance check fails for Mock objects with spec executed under tracing function

[nedbat]

BPO	36593
Nosy	@nedbat, @cjw296, @voidspace, @mariocj89, @pablogsal, @miss-islington, @tirkarthi
PRs	bpo-36593: Fix isinstance check for Mock objects with spec executed under tracing #12790 [3.7] bpo-36593: Fix isinstance check for Mock objects with spec executed under tracing (GH-12790) #12821
Files	traced_sscce.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-04-14.02:48:46.917>
created_at = <Date 2019-04-11.00:04:16.787>
labels = ['3.7', '3.8', 'type-bug', 'library']
title = 'isinstance check fails for Mock objects with spec executed under tracing function'
updated_at = <Date 2019-04-14.02:48:46.915>
user = 'https://github.com/nedbat'

bugs.python.org fields:

activity = <Date 2019-04-14.02:48:46.915>
actor = 'pablogsal'
assignee = 'none'
closed = True
closed_date = <Date 2019-04-14.02:48:46.917>
closer = 'pablogsal'
components = ['Library (Lib)']
creation = <Date 2019-04-11.00:04:16.787>
creator = 'nedbat'
dependencies = []
files = ['48260']
hgrepos = []
issue_num = 36593
keywords = ['patch', '3.7regression']
message_count = 11.0
messages = ['339903', '339904', '339908', '339909', '339956', '339964', '339987', '340170', '340171', '340172', '340177']
nosy_count = 7.0
nosy_names = ['nedbat', 'cjw296', 'michael.foord', 'mariocj89', 'pablogsal', 'miss-islington', 'xtreak']
pr_nums = ['12790', '12821']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue36593'
versions = ['Python 3.7', 'Python 3.8']

[nedbat]

In Python 3.7.3, having a trace function in effect while mock is imported causes isinstance to be wrong for MagicMocks. I know, this sounds unlikely...

$ cat traced_sscce.py
# sscce.py
import sys

def trace(frame, event, arg):
    return trace

if len(sys.argv) > 1:
    sys.settrace(trace)

from unittest.mock import MagicMock

class A:
    pass

m = MagicMock(spec=A)
print("isinstance: ", isinstance(m, A))

$ python3.7.2 traced_sscce.py
isinstance:  True

$ python3.7.2 traced_sscce.py 1
isinstance:  True

$ python3.7.2 -VV
Python 3.7.2 (default, Feb 17 2019, 16:54:12)
[Clang 10.0.0 (clang-1000.10.44.4)]

$ python3.7.3 traced_sscce.py
isinstance:  True

$ python3.7.3 traced_sscce.py 1
isinstance:  False

$ python3.7.3 -VV
Python 3.7.3 (default, Apr 10 2019, 10:27:53)
[Clang 10.0.0 (clang-1000.10.44.4)]

Note that if you move the mock import to before the settrace call, everything works fine.

[nedbat]

This also affects Python 3.8.0a3

[tirkarthi]

There are very few changes to mock.py on bisecting with the example in the report is d358a8c the problem? I am surprised by the reason this commit leads to change in behavior with set trace.

➜ cpython git:(master) ✗ cat ../backups/bpo36593.py
import sys

def trace(frame, event, arg):
    return trace

if len(sys.argv) > 1:
    sys.settrace(trace)

from unittest.mock import MagicMock

class A:
    pass

m = MagicMock(spec=A)
print("isinstance: ", isinstance(m, A))
➜  cpython git:(master) ✗ git checkout d358a8cda75446a8e0b5d99149f709395d5eae19 Lib/unittest/mock.py
➜  cpython git:(master) ✗ ./python.exe ../backups/bpo36593.py 1
isinstance:  False
➜  cpython git:(master) ✗ git checkout d358a8cda75446a8e0b5d99149f709395d5eae19~1 Lib/unittest/mock.py
➜  cpython git:(master) ✗ ./python.exe ../backups/bpo36593.py 1
isinstance:  True

commit d358a8c
Author: Miss Islington (bot) <31488909+miss-islington@users.noreply.github.com>
Date: Mon Jan 21 01:37:54 2019 -0800

bpo-20239: Allow repeated deletion of unittest.mock.Mock attributes (GH-11629)

* Allow repeated deletion of unittest.mock.Mock attributes

* fixup! Allow repeated deletion of unittest.mock.Mock attributes

* fixup! fixup! Allow repeated deletion of unittest.mock.Mock attributes
(cherry picked from commit 222d303ade8aadf0adcae5190fac603bdcafe3f0)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

[tirkarthi]

On an initial guess this is the change that went through and using object.__delattr__(self, name) instead of super().__delattr__(name) restores the old behavior and there are no test case failures. But I still have to check the change and how it's related to set trace.

diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py
index 955af5d2b8..42fbc22e74 100644
--- a/Lib/unittest/mock.py
+++ b/Lib/unittest/mock.py
@@ -728,11 +728,10 @@ class NonCallableMock(Base):
# not set on the instance itself
return

-        if name in self.__dict__:
-            object.__delattr__(self, name)
-
         obj = self._mock_children.get(name, _missing)
-        if obj is _deleted:
+        if name in self.__dict__:
+            super().__delattr__(name)
+        elif obj is _deleted:
             raise AttributeError(name)
         if obj is not _missing:
             del self._mock_children[name]

[nedbat]

BTW, this started as a bug against coverage.py: coveragepy/coveragepy#795

There are some more details there, including the fact that it must be a Python trace function; a C trace function seems not to cause the problem.

[tirkarthi]

I think I got to this. See bpo-12370 (Use of super overwrites use of __class__ in class namespace) . msg138712 suggests a workaround to alias super at the module level. unittest.mock has the below line and NonCallableMock has __class__ defined as a property which is not set when set trace is used since the linked commit in 3.7.2 uses super() is somehow executed by settrace since it's in __delattr__ and not in a code path executed by user code?

Looking at the unittest.mock code super is not used anywhere but _safe_super is used which I hope is for this reason. So this is not a case with MagicMock only but rather a case with anything that inherits from NonCallableMock i.e. Mock, MagicMock etc. This is not a problem with import being made before the trace is set as you mentioned so set trace does something that exposes this issue

cpython/Lib/unittest/mock.py

Line 48 in 536a35b

# Workaround for issue #12370

# Workaround for issue python/cpython#56579
# Without this, the __class__ properties wouldn't be set correctly
_safe_super = super

Sample program in bpo-12370. Add set trace SuperClass with using super() fails and passes without set trace for super(). Whereas SafeSuperClass that uses _safe_super (module level alias to super) passes both cases irrespective of usage of set trace. Nick coughlan added a message so that unittest.mock should use _safe_super in https://bugs.python.org/issue12370#msg161704

import sys

_safe_super = super

def trace(frame, event, arg):
    return trace

if len(sys.argv) > 1:
    sys.settrace(trace)

class SuperClass(object):

    def __init__(self):
        super().__init__()

    @property
    def __class__(self):
        return int

class SafeSuperClass(object):

    def __init__(self):
        _safe_super(SafeSuperClass, self).__init__()

    @property
    def __class__(self):
        return int

print(isinstance(SuperClass(), int))
print(isinstance(SafeSuperClass(), int))

Running above code with trace and without trace

➜ cpython git:(master) ✗ ./python.exe /tmp/buz.py
True
True
➜ cpython git:(master) ✗ ./python.exe /tmp/buz.py 1
False
True

There is a test for the above in Lib/test/test_super.py at

cpython/Lib/test/test_super.py

Line 87 in 4c409be

def test_various___class___pathologies(self):

Add a trace as below in test_super.py at the top and the test case fails

import sys

def trace(frame, event, arg):
    return trace

sys.settrace(trace)

➜ cpython git:(master) ✗ ./python.exe Lib/test/test_super.py
....................F
======================================================================
FAIL: test_various___class___pathologies (main.TestSuper)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "Lib/test/test_super.py", line 100, in test_various___class___pathologies
    self.assertEqual(x.__class__, 413)
AssertionError: <class '__main__.TestSuper.test_various___class___pathologies.<locals>.X'> != 413

---

Ran 21 tests in 0.058s

FAILED (failures=1)

The patch for this issue would be to use _safe_super instead of super() or to fallback to object() in the previous case that doesn't seemed to have suffered from this case. I can try to make a PR with tests. Added Michael for confirmation.

diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py
index 8684f1dfa5..0e77f0e489 100644
--- a/Lib/unittest/mock.py
+++ b/Lib/unittest/mock.py
@@ -739,7 +739,7 @@ class NonCallableMock(Base):

         obj = self._mock_children.get(name, _missing)
         if name in self.__dict__:
-            super().__delattr__(name)
+            _safe_super(NonCallableMock, self).__delattr__(name)
         elif obj is _deleted:
             raise AttributeError(name)
         if obj is not _missing:

[tirkarthi]

Copying my comment from GitHub :

This issue occurs only when the mock module import and code are executed under tracing. In order to test this I have tried patching sys.modules to remove unittest.mock in order to trigger an import under tracing. Though unittest.mock is deleted reference to unittest.mock.patch is held by two tests (test_patch_dict_test_prefix and test_patch_test_prefix) that change the TEST_PREFIX. Hence somehow there is a difference in the unittest.mock.patch object referred under these tests and inside mock module causing reference to unchanged TEST_PREFIX and hence test case failures. So I have kept a reference to old patch object and restored it in the end of this test to make sure everything is in sync.

There is some difficulty in testing this since the import needs to be executed under tracing and manipulating sys.modules to delete unittest.mock and reimport causes some old references in other tests to go out of sync. A clean way to do this would be to have this test as a separate file and hence not impacting other tests but I am not sure if it's worth enough to justify a separate file to test this. Maybe I am missing something here on a better way to test this. Any guidance would be helpful.

Thanks

[pablogsal]

New changeset 830b43d by Pablo Galindo (Xtreak) in branch 'master':
bpo-36593: Fix isinstance check for Mock objects with spec executed under tracing (GH-12790)
830b43d

[miss-islington]

New changeset f3a9d72 by Miss Islington (bot) in branch '3.7':
bpo-36593: Fix isinstance check for Mock objects with spec executed under tracing (GH-12790)
f3a9d72

[tirkarthi]

An interesting thing is that there is GCC in CI that runs under coverage which also passed since this requires a Python tracing function before importing mock to trigger this as in the original report. Adding a simple tracer at the top of testmock.py file does cause an instance check failure without patch and passes with the merged patch.

$ ./python.exe Lib/unittest/test/testmock/testmock.py
.......................................F.................s..............................................F.......

======================================================================
FAIL: test_class_assignable (main.MockTest)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "Lib/unittest/test/testmock/testmock.py", line 1828, in test_class_assignable
    self.assertIsInstance(mock, int)
AssertionError: <Mock spec='int' id='4405923920'> is not an instance of <class 'int'>

======================================================================
FAIL: test_spec_class (main.MockTest)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "Lib/unittest/test/testmock/testmock.py", line 775, in test_spec_class
    self.assertIsInstance(mock, X)
AssertionError: <Mock spec='X' id='4405437744'> is not an instance of <class '__main__.MockTest.test_spec_class.<locals>.X'>

---

Ran 112 tests in 3.834s

FAILED (failures=2, skipped=1)