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

Disallow Mock spec arguments from being Mocks #87644

Open
msuozzo mannequin opened this issue Mar 12, 2021 · 10 comments
Open

Disallow Mock spec arguments from being Mocks #87644

msuozzo mannequin opened this issue Mar 12, 2021 · 10 comments
Labels
3.10 only security fixes tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@msuozzo
Copy link
Mannequin

msuozzo mannequin commented Mar 12, 2021

BPO 43478
Nosy @gpshead, @vstinner, @cjw296, @voidspace, @lisroach, @mariocj89, @pablogsal, @tirkarthi, @msuozzo
PRs
  • bpo-43478: Restrict use of Mock objects as specs #25326
  • bpo-43478: Fix formatting of NEWS entry #25335
  • bpo-43753: Add Py_Is() and Py_IsNone() functions #25227
  • bpo-43478: Restrict use of Mock objects as specs #31090
  • 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 2021-03-12.01:53:43.131>
    labels = ['type-feature', 'tests', '3.10']
    title = 'Disallow Mock spec arguments from being Mocks'
    updated_at = <Date 2022-02-02.23:26:50.979>
    user = 'https://bugs.python.org/msuozzo'

    bugs.python.org fields:

    activity = <Date 2022-02-02.23:26:50.979>
    actor = 'matthew.suozzo'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Tests']
    creation = <Date 2021-03-12.01:53:43.131>
    creator = 'msuozzo'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 43478
    keywords = ['patch']
    message_count = 10.0
    messages = ['388532', '388753', '388755', '388766', '388767', '388769', '388878', '388880', '390690', '390743']
    nosy_count = 10.0
    nosy_names = ['gregory.p.smith', 'vstinner', 'cjw296', 'michael.foord', 'lisroach', 'mariocj89', 'pablogsal', 'xtreak', 'msuozzo', 'matthew.suozzo']
    pr_nums = ['25326', '25335', '25227', '31090']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue43478'
    versions = ['Python 3.10']

    @msuozzo
    Copy link
    Mannequin Author

    msuozzo mannequin commented Mar 12, 2021

    An unfortunately common pattern over large codebases of Python tests is for spec'd Mock instances to be provided with Mock objects as their specs. This gives the false sense that a spec constraint is being applied when, in fact, nothing will be disallowed.

    The two most frequently observed occurrences of this anti-pattern are as follows:

    • Re-patching an existing autospec.
      def setUp(self):
        mock.patch.object(mod, 'Klass', autospec=True).start()
        self.addCleanup(mock.patch.stopall)

    @mock.patch.object(mod, 'Klass', autospec=True) # :(
    def testFoo(self, mock_klass):
    # mod.Klass has no effective spec.

    • Deriving an autospec Mock from an already-mocked object
      def setUp(self):
        mock.patch.object(mod, 'Klass').start()
        ...
        mock_klass = mock.create_autospec(mod.Klass)  # :(
        # mock_klass has no effective spec

    This is fairly easy to detect using _is_instance_mock at patch time however it can break existing tests.

    I have a patch ready and it seems like this error case is not frequent enough that it would be disruptive to address.

    Another option would be add it as a warning for a version e.g. 3.10 and then potentially make it a breaking change in 3.11. However considering this is a test-only change with a fairly clear path to fix it, that might be overly cautious.

    @msuozzo msuozzo mannequin added 3.10 only security fixes tests Tests in the Lib/test dir type-feature A feature request or enhancement labels Mar 12, 2021
    @gpshead
    Copy link
    Member

    gpshead commented Mar 15, 2021

    Lets go ahead and try making this a breaking change in 3.10. If users report it causes a bunch of problems during the beta -that they don't want to address so soon- (they are all likely bugs in test suites...) we can soften it to a warning for a cycle. My hope is that we won't need to do that.

    @tirkarthi
    Copy link
    Member

    For a simple experiment raising an exception I can see two tests failing in test suite that have the pattern of having an autospec which is a mock object.

    diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py
    index 720f682efb..d33c7899a1 100644
    --- a/Lib/unittest/mock.py
    +++ b/Lib/unittest/mock.py
    @@ -2612,6 +2612,9 @@ def create_autospec(spec, spec_set=False, instance=False, _parent=None,
             # interpreted as a list of strings
             spec = type(spec)
     
    +    if _is_instance_mock(spec):
    +        1 / 0
    +        
         is_type = isinstance(spec, type)
         is_async_func = _is_async_func(spec)
         _kwargs = {'spec': spec}

    ./python -m unittest Lib.unittest.test.testmock
    ....................................................................................................................................................E..............................................................................................................................................................E...........................................................................................................................................................................................
    ======================================================================
    ERROR: test_bool_not_called_when_passing_spec_arg (unittest.test.testmock.testmock.MockTest)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/root/cpython/Lib/unittest/test/testmock/testmock.py", line 2180, in test_bool_not_called_when_passing_spec_arg
        with unittest.mock.patch.object(obj, 'obj_with_bool_func', autospec=True): pass
      File "/root/cpython/Lib/unittest/mock.py", line 1503, in __enter__
        new = create_autospec(autospec, spec_set=spec_set,
      File "/root/cpython/Lib/unittest/mock.py", line 2616, in create_autospec
        1 / 0
    ZeroDivisionError: division by zero

    ======================================================================
    ERROR: test_create_autospec_awaitable_class (unittest.test.testmock.testasync.AsyncAutospecTest)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/root/cpython/Lib/unittest/test/testmock/testasync.py", line 204, in test_create_autospec_awaitable_class
        self.assertIsInstance(create_autospec(awaitable_mock), AsyncMock)
      File "/root/cpython/Lib/unittest/mock.py", line 2616, in create_autospec
        1 / 0
    ZeroDivisionError: division by zero

    Ran 495 tests in 2.039s

    FAILED (errors=2)

    @cjw296
    Copy link
    Contributor

    cjw296 commented Mar 15, 2021

    I agree that this should raise an exception.
    Can the two failing tests in mock's own suite be easily fixed?

    @msuozzo
    Copy link
    Mannequin Author

    msuozzo mannequin commented Mar 15, 2021

    I've fixed a bunch of these in our internal repo so I'd be happy to add that to a patch implementing raising exceptions for these cases.

    @tirkarthi
    Copy link
    Member

    The tests can be fixed. The change to raise exception can be merged to gather feedback during alpha/beta and see if there are any valid usecases.

    @msuozzo
    Copy link
    Mannequin Author

    msuozzo mannequin commented Mar 16, 2021

    A few more things:

    Assertions on Mock-autospec'ed Mocks will silently pass since e.g. assert_called_once_with will now be mocked out. This may justify a more stringent stance on the pattern since it risks hiding real test failures.

    One complicating factor with the implementation is that autospec'd objects may have children that are already Mocked out. Failing eagerly, however, would break cases where the child is never used (and consequently risk causing more friction than may be necessary) but failing only upon access of that child makes it trickier for the user to trace back to where the parent was mocked.

    @msuozzo
    Copy link
    Mannequin Author

    msuozzo mannequin commented Mar 16, 2021

    And to give some context for the above autospec child bit, this is the relevant code that determines the spec to use for each child: https://github.com/python/cpython/blob/master/Lib/unittest/mock.py#L2671-L2696

    @gpshead
    Copy link
    Member

    gpshead commented Apr 10, 2021

    New changeset dccdc50 by Matthew Suozzo in branch 'master':
    bpo-43478: Restrict use of Mock objects as specs (GH-25326)
    dccdc50

    @pablogsal
    Copy link
    Member

    New changeset 6e468cb by Pablo Galindo in branch 'master':
    bpo-43478: Fix formatting of NEWS entry (GH-25335)
    6e468cb

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    farirat added a commit to jookies/jasmin that referenced this issue Dec 9, 2022
    Ref: python/cpython#87644
    Signed-off-by: farirat <fourat@gmail.com>
    openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Dec 12, 2022
    * Update ironic from branch 'master'
      to f6bc139c6dcfa877051f59e5ce354f72fa07d8e9
      - Merge "Fix unit tests for Python 3.11"
      - Fix unit tests for Python 3.11
        
        Mocks can no longer be provided as the specs for other Mocks.
        See python/cpython#87644 and
        https://docs.python.org/3.11/whatsnew/3.11.html for more info.
        
        Change-Id: If7c10d9bfd0bb410b3bc5180b737439c92e515da
    openstack-mirroring pushed a commit to openstack/ironic that referenced this issue Dec 12, 2022
    Mocks can no longer be provided as the specs for other Mocks.
    See python/cpython#87644 and
    https://docs.python.org/3.11/whatsnew/3.11.html for more info.
    
    Change-Id: If7c10d9bfd0bb410b3bc5180b737439c92e515da
    openstack-mirroring pushed a commit to openstack/ironic that referenced this issue Dec 16, 2022
    Mocks can no longer be provided as the specs for other Mocks.
    See python/cpython#87644 and
    https://docs.python.org/3.11/whatsnew/3.11.html for more info.
    
    Change-Id: If7c10d9bfd0bb410b3bc5180b737439c92e515da
    (cherry picked from commit 342f4b3)
    openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Feb 2, 2023
    * Update neutron from branch 'master'
      to dc0b1467d6fc79e0b3f0a0ab15c94eebf52b3176
      - Merge "Add tox Python3.11 job to the testing queues"
      - Add tox Python3.11 job to the testing queues
        
        This new job called "tox-py311" is added to the following
        template queues:
        * check
        * periodic
        
        This patch tries to address what has been detected in [1][2]: a
        mock spec argument cannot be another mock.
        
        This new job is temporary. Once the "openstack-tox-py311" job
        definition is created and added to the "openstack-python3-jobs"
        template, we'll revert this patch. Until this happens, we'll
        test Python3.11 using the "tox-py311" job defined in [3].
        
        [1]python/cpython#87644
        [2]https://bugs.launchpad.net/cinder/+bug/2000436
        [3]https://opendev.org/zuul/zuul-jobs/src/branch/master/zuul.d/python-jobs.yaml
        
        Change-Id: Iebe3fc75ca8b15cec49603e61a7acd211f24e03e
    openstack-mirroring pushed a commit to openstack/neutron that referenced this issue Feb 2, 2023
    This new job called "tox-py311" is added to the following
    template queues:
    * check
    * periodic
    
    This patch tries to address what has been detected in [1][2]: a
    mock spec argument cannot be another mock.
    
    This new job is temporary. Once the "openstack-tox-py311" job
    definition is created and added to the "openstack-python3-jobs"
    template, we'll revert this patch. Until this happens, we'll
    test Python3.11 using the "tox-py311" job defined in [3].
    
    [1]python/cpython#87644
    [2]https://bugs.launchpad.net/cinder/+bug/2000436
    [3]https://opendev.org/zuul/zuul-jobs/src/branch/master/zuul.d/python-jobs.yaml
    
    Change-Id: Iebe3fc75ca8b15cec49603e61a7acd211f24e03e
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    Status: No status
    Development

    No branches or pull requests

    4 participants