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

unittest.mock spec calls class properties #85934

Open
melwitt mannequin opened this issue Sep 12, 2020 · 4 comments
Open

unittest.mock spec calls class properties #85934

melwitt mannequin opened this issue Sep 12, 2020 · 4 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@melwitt
Copy link
Mannequin

melwitt mannequin commented Sep 12, 2020

BPO 41768
Nosy @terryjreedy, @cjw296, @voidspace, @ambv, @lisroach, @mariocj89, @tirkarthi, @sobolevn, @melwitt
PRs
  • gh-85934: Use getattr_static when adding mock spec #22209
  • 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 = None
    created_at = <Date 2020-09-12.01:10:24.232>
    labels = ['3.8', 'type-bug', 'library', '3.9', '3.10']
    title = 'unittest.mock spec calls class properties'
    updated_at = <Date 2021-12-03.08:45:33.635>
    user = 'https://github.com/melwitt'

    bugs.python.org fields:

    activity = <Date 2021-12-03.08:45:33.635>
    actor = 'AlexWaygood'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2020-09-12.01:10:24.232>
    creator = 'melwitt'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41768
    keywords = ['patch']
    message_count = 3.0
    messages = ['376757', '377140', '407569']
    nosy_count = 9.0
    nosy_names = ['terry.reedy', 'cjw296', 'michael.foord', 'lukasz.langa', 'lisroach', 'mariocj89', 'xtreak', 'sobolevn', 'melwitt']
    pr_nums = ['22209']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue41768'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @melwitt
    Copy link
    Mannequin Author

    melwitt mannequin commented Sep 12, 2020

    When async magic method support was added to unittest.mock.Mock to address issue bpo-26467, it introduced a getattr call [1] that causes class properties to be called when the class is used as a mock spec.

    This caused a problem for a test in my project when running with Python 3.8 where previously the test worked OK with Python 3.6.

    The test aims to verify that a class instance is not created if the called code path does not access the class property and thus the class will not create a heavy object unless it's needed (lazy create on access via @Property).

    As of Python 3.8, the @Property is always called and is called by the mock spec process itself, even though the code path being tested does not access the class @Property.

    Here is a code snippet that illustrates the @Property calling from the mock spec alone:

    class SomethingElse(object):
        def __init__(self):
            self._instance = None
    
        @property
        def instance(self):
            if not self._instance:
                self._instance = 'object'

    ...

        def test_property_not_called_with_spec_mock(self):
            obj = SomethingElse()
            self.assertIsNone(obj._instance)
            mock = Mock(spec=obj)
            self.assertIsNone(obj._instance)
    $ ./python -m unittest -v unittest.test.testmock.testmock.MockTest.test_property_not_called_with_spec_mock
    test_property_not_called_with_spec_mock (unittest.test.testmock.testmock.MockTest) ... FAIL
    
    
    ======================================================================
    FAIL: test_property_not_called_with_spec_mock (unittest.test.testmock.testmock.MockTest)
    \----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/vagrant/cpython/Lib/unittest/test/testmock/testmock.py", line 2173, in test_property_not_called_with_spec_mock
        self.assertIsNone(obj._instance)
    AssertionError: 'object' is not None

    [1]

    if iscoroutinefunction(getattr(spec, attr, None)):

    @melwitt melwitt mannequin added 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Sep 12, 2020
    @terryjreedy
    Copy link
    Member

    Without lines numbers, I cannot test which of the two identical asserts failed. Either comments or msg arguments will differentiate. Nor can I run snippets from two different files. Here is a minimal reproducible self-contained code that demonstrates the claim (which I verified on 3.9 and current master).

    import unittest
    from unittest.mock import Mock
    
    class SomethingElse(object):
        def __init__(self):
            self._instance = None
    
        @property
        def instance(self):
            if not self._instance:
                self._instance = 'object'
    
    class Test(unittest.TestCase):
    
        def test_property_not_called_with_spec_mock(self):
            obj = SomethingElse()
            self.assertIsNone(obj._instance, msg='before') # before
            mock = Mock(spec=obj)
            self.assertIsNone(obj._instance, msg='after') # after
    
    unittest.main()

    @terryjreedy terryjreedy added 3.9 only security fixes 3.10 only security fixes labels Sep 18, 2020
    @sobolevn
    Copy link
    Member

    sobolevn commented Dec 3, 2021

    Related PR: #29901

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @f0k
    Copy link

    f0k commented Feb 22, 2023

    I'm affected by the same problem: The iscoroutinefunction(getattr(spec, attr, None)) call to search for coroutines triggers properties that were not meant to be triggered (and were not triggered in earlier versions of Python). This issue is also reported in #89917 and #98301. In my use case, I am mocking a base class that raises NotImplementedError on some properties which are not involved in the unit test anyway. I'd need to mock a derived class instead that has all properties implemented, but that feels wrong given that I want to test aspects of the base class.

    There are three PRs and one issue proposing different solutions:

    1. gh-85934: Use getattr_static when adding mock spec #22209: uses inspect.getattr_static instead of getattr. It was not merged because the sole reviewer was not confident that they could judge whether the change was correct.
    2. bpo-45756: do not execute @property descrs while creating mock autospecs #29901: uses inspect.getattr_static to check if the attribute is a property, and skips the iscoroutinefunction(getattr(...)) call in this case. It was not merged because it missed a core review. Raymond Hettinger left a comment asking whether some use cases might rely on all properties being called.
    3. bpo-45756: Fix mock triggers dynamic lookup via the descriptor protocol. #31348: iterates over inspect.getmembers_static(spec, iscoroutinefunction) instead of iterating over all attributes. It was asked whether async classmethods would still be caught.
    4. unittest.mock.create_autospec() can crash when accessing properties from objects #98301: proposes to wrap getattr in a try/except block to silence any occurring exceptions. It did not receive any feedback.

    All solutions would work for the case depicted here. There is a corner case that is treated differently between the current code and the first three proposals:

    class X(object):
        @property
        def foo():
            async def bar():
                pass
            return bar

    If we spec an instance of this class, X(), the current code will add foo to the _spec_asyncs list, since it returns a coroutine function. The first three proposals will not add foo because they get to see the property instance instead of the result of the property's getter. I'm not sure if this is relevant.

    The fourth solution will handle this corner case as the current code, but does not fix the issue discussed in this post, which was about avoiding expensive lazy initialization of objects triggered by a property getter.

    I would suggest to go for the fix in #22209, and include the additional test case from #31348.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    Status: No status
    Development

    No branches or pull requests

    3 participants