Mock autospec not used, or not used properly

Bug #1735588 reported by Claudiu Belu on 2017-12-01
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
Undecided
Stephen Finucane
compute-hyperv
Undecided
Unassigned
networking-hyperv
Undecided
Claudiu Belu
os-win
Undecided
Claudiu Belu
oslotest
Undecided
Claudiu Belu

Bug Description

Description
===========

In typical unit tests, almost all of the dependencies are mocked or patched (mock.patch), without any guarantee that the mocked methods actually exist, or if their signatures are respected (see below). Because of this, actual issues can easily be overlooked and missed, as the unit tests are wrongfully passing. [0]

The mock.Mock class accepts a spec as an argument, which only solves half the problem: it only checks if an attribute exists, based on the given spec. It does not guarantee that the given attribute is actually a method, or if its signature is respected [1][2]. Some unit tests may pass the autospec argument, but mock doesn't support it at the moment.

mock.patch, mock.patch.object, mock.patch.multiple accept an autospec argument, but because of a bug [3][4], it cannot be used properly.

Steps to reproduce
==================

import mock
from mock.tests import testmock

m = mock.Mock(spec=testmock.Something)

# meth has the following signature:
# def meth(self, a, b, c, d=None):
m.meth()

Expected result
===============

TypeError should be raised.

Actual result
=============

A mock object is returned.

Proposal
========

Bug reports have been issues for the bugs mentioned above, see [1][2][3][4], but until then, the mock library can be patched to include the following changes:

- add autospec argument to mock.Mock and mock.MagicMock. Unit test should then pass the autospec argument whenever possible.
- fix the mock.patch autospec issue.
- enable mock.patch autospec by default, unless otherwise specified.

Links
=====

[0] https://review.openstack.org/#/c/461689/
[1] https://github.com/testing-cabal/mock/issues/393
[2] https://bugs.python.org/issue30587
[3] https://github.com/testing-cabal/mock/issues/396
[4] https://bugs.python.org/issue32092

Claudiu Belu (cbelu) on 2017-12-01
Changed in nova:
assignee: nobody → Claudiu Belu (cbelu)
Changed in networking-hyperv:
assignee: nobody → Claudiu Belu (cbelu)
Changed in os-win:
assignee: nobody → Claudiu Belu (cbelu)
Changed in oslotest:
assignee: nobody → Claudiu Belu (cbelu)

Fix proposed to branch: master
Review: https://review.openstack.org/524438

Changed in oslotest:
status: New → In Progress
Changed in nova:
status: New → In Progress
Changed in nova:
assignee: Claudiu Belu (cbelu) → Stephen Finucane (stephenfinucane)
Changed in nova:
assignee: Stephen Finucane (stephenfinucane) → Claudiu Belu (cbelu)

Reviewed: https://review.openstack.org/524438
Committed: https://git.openstack.org/cgit/openstack/oslotest/commit/?id=0bde35899a10875891f0592961c01e9fbb854738
Submitter: Zuul
Branch: master

commit 0bde35899a10875891f0592961c01e9fbb854738
Author: Claudiu Belu <email address hidden>
Date: Fri Dec 1 03:24:14 2017 +0200

    Adds mock autospec fixture

    In typical unit tests, almost all of the dependencies are mocked or
    patched (mock.patch), without any guarantee that the mocked methods
    actually exist, or if their signatures are respected (see below). Because
    of this, actual issues can easily be overlooked and missed, as the unit
    tests are wrongfully passing.

    The mock.Mock class accepts a spec as an argument, which only solves half
    the problem: it only checks if an attribute exists, based on the given
    spec. It does not guarantee that the given attribute is actually a method,
    or if its signature is respected. Some unit tests may pass the autospec
    argument, but mock doesn't support it at the moment.

    mock.patch, mock.patch.object, mock.patch.multiple accept an autospec
    argument, but because of a bug, it cannot be used properly.

    Adds a fixture which replaces mock.Mock and mock.MagicMock with
    subclass which accepts the autospec argument, and on call, it will
    check the signature of the called method / function.

    Adds a function which replaces mock.mock._patch with a subclass, which
    treats the autospec argument properly (consumes the self / cls argument),
    and sets autospec=True by default, unless otherwise specified.
    WARNING: this function is not a fixture, and in order to benefit from it,
    it will have to be called as EARLY as possible, before any test classes
    are loaded, otherwise the original mock.mock._patch is used instead.

    Needed-By: I3636833962c905faa0f144c7fdc4833037324d31
    Needed-By: I4484e63c97bd1cdde3d88855eabe7545784f365e

    Closes-Bug: #1735588

    Change-Id: I0e4a55fbf4c1d175726ca22b664e240849a99856

Changed in oslotest:
status: In Progress → Fix Released

This issue was fixed in the openstack/oslotest 3.2.0 release.

Changed in os-win:
status: New → In Progress

Fix proposed to branch: master
Review: https://review.openstack.org/532947

Changed in networking-hyperv:
status: New → In Progress

Reviewed: https://review.openstack.org/533230
Committed: https://git.openstack.org/cgit/openstack/oslotest/commit/?id=5d3196781bc3b51eee04f4ed7a77af0c9fdbcaba
Submitter: Zuul
Branch: master

commit 5d3196781bc3b51eee04f4ed7a77af0c9fdbcaba
Author: Claudiu Belu <email address hidden>
Date: Tue Jan 9 21:46:45 2018 -0800

    mock: Fixes mock.patch.multiple autospec

    mock.patch.multiple causes the patch result to be a dictionary,
    which causes the autospec to fail. This patch will autospec
    properly in this scenario as well.

    Change-Id: I3aa97459bf53059422c8fa696cb09df811c1701b
    Related-Bug: #1735588

Fix proposed to branch: master
Review: https://review.openstack.org/535806

Changed in nova:
assignee: Claudiu Belu (cbelu) → Matthew Edmonds (edmondsw)

Reviewed: https://review.openstack.org/532619
Committed: https://git.openstack.org/cgit/openstack/os-win/commit/?id=823b162c6a02083f6ae6f6ce27150c039687993d
Submitter: Zuul
Branch: master

commit 823b162c6a02083f6ae6f6ce27150c039687993d
Author: Claudiu Belu <email address hidden>
Date: Wed Jan 10 21:47:21 2018 +0200

    tests: Use mock autospec in unit tests

    Autospecing will ensure that the method signatures are respected
    during calls.

    oslotest.mock_fixture contains 2 components, one of them adds the autospec
    argument to mock.Mock and mock.MagicMock, while the other one fixes the
    autospec behaviour for mock.patch functions, and sets autospec to True by
    default, unless otherwise specified.

    Closes-Bug: #1735588

    Change-Id: I5a1dd8571988859b4a14a505fd5e016079582363

Changed in os-win:
status: In Progress → Fix Released

Reviewed: https://review.openstack.org/532947
Committed: https://git.openstack.org/cgit/openstack/networking-hyperv/commit/?id=d5b72f4250ce3a60fb85f751c99fbba88a71ced0
Submitter: Zuul
Branch: master

commit d5b72f4250ce3a60fb85f751c99fbba88a71ced0
Author: Claudiu Belu <email address hidden>
Date: Tue Jan 9 05:53:42 2018 -0800

    tests: Use mock autospec in unit tests

    Autospecing will ensure that the method signatures are respected
    during calls.

    oslotest.mock_fixture contains 2 components, one of them adds the autospec
    argument to mock.Mock and mock.MagicMock, while the other one fixes the
    autospec behaviour for mock.patch functions, and sets autospec to True by
    default, unless otherwise specified.

    Change-Id: Ib785be16e2b81a32dff32b0e070e34e0675d4b4f
    Closes-Bug: #1735588

Changed in networking-hyperv:
status: In Progress → Fix Released
Changed in nova:
assignee: Matthew Edmonds (edmondsw) → Claudiu Belu (cbelu)

Change abandoned by Matthew Edmonds (<email address hidden>) on branch: master
Review: https://review.openstack.org/535806
Reason: abandoning in favor of https://review.openstack.org/#/c/447505

This issue was fixed in the openstack/networking-hyperv 6.0.0 release.

Reviewed: https://review.openstack.org/539908
Committed: https://git.openstack.org/cgit/openstack/compute-hyperv/commit/?id=26c91401ee8f203d74261a6b0c67196173ef57d6
Submitter: Zuul
Branch: master

commit 26c91401ee8f203d74261a6b0c67196173ef57d6
Author: Claudiu Belu <email address hidden>
Date: Tue Jan 30 03:48:37 2018 -0800

    tests: autospecs mock.patch and classes usages

    By default, mock.patch's autospec argument is None, meaning that
    there's no signature checking for the patched methods and funtions.

    oslotest.mock_fixture.patch_mock_module fixes a few issues within
    mock.patch functions, as well as setting autospec=True by default,
    unless otherwise specified or new_callable, create, spec arguments
    are passed in.

    Updates HyperVBaseTestCase to autospec the classes used by different
    *Ops classes.

    Change-Id: Ifbdc185d0b50e29c5d5f3ff036141253d2323076
    Partial-Bug: #1735588

This issue was fixed in the openstack/os-win 4.0.0 release.

Reviewed: https://review.openstack.org/538022
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=9c7d6123e3e66fd8b182af548273df9e7d66ee36
Submitter: Zuul
Branch: master

commit 9c7d6123e3e66fd8b182af548273df9e7d66ee36
Author: Claudiu Belu <email address hidden>
Date: Wed Jan 24 23:30:37 2018 -0800

    tests: refactors and cleans up test_rbd.py

    In the unit tests' setUp, it was setting self.mock_rbd = mock_rbd
    (where mock_rbd is the argument passed by @mock.patch.object) and
    setting all sorts of properties onto self.mock_rbd, but mock_rbd
    would become invalid after setUp finished, as the mock.patch
    decorator would remove the patched mock. The same applies with
    mock_rados.

    This patch addresses this issue by refactoring and simplifying the
    unit tests.

    Change-Id: Ie2bf4d559e5391be3fe778835673a11de7ec8e1b
    Related-Bug: #1735588

Reviewed: https://review.openstack.org/539954
Committed: https://git.openstack.org/cgit/openstack/compute-hyperv/commit/?id=d19606a82809f2a909d20b102166880b65d74b1f
Submitter: Zuul
Branch: master

commit d19606a82809f2a909d20b102166880b65d74b1f
Author: Claudiu Belu <email address hidden>
Date: Tue Jan 30 06:06:47 2018 -0800

    autospecs os-win utils classes

    Most of the os-win utils are created through
    os_win.utilsfactory, which all rely on its function
    '_get_class' to load and initialize the proper Utils
    based on the OS version.

    This commit patches the mention method, returning
    an autospeced mock for the desired Utils instead.

    Closes-Bug: #1735588

    Change-Id: If0002b2c711aaeaf13273bed9b912fdd9c639a64

Changed in compute-hyperv:
status: New → Fix Released

Reviewed: https://review.openstack.org/447505
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=1f0638f943a7b490db3cf9a60423ebae859682cc
Submitter: Zuul
Branch: master

commit 1f0638f943a7b490db3cf9a60423ebae859682cc
Author: Claudiu Belu <email address hidden>
Date: Mon Mar 20 15:34:37 2017 +0200

    tests: fixes mock autospec usage

    Until now, mock.Mock did not receive any autospec argument, yet
    there are a few tests which are using it.

    The autospec argument is being added by a fixture, and some unit
    tests are not using it properly.

    Depends-On: I0e4a55fbf4c1d175726ca22b664e240849a99856

    Partial-Bug: #1735588

    Change-Id: I3636833962c905faa0f144c7fdc4833037324d31

Reviewed: https://review.openstack.org/557368
Committed: https://git.openstack.org/cgit/openstack/oslotest/commit/?id=bb78b84c3fc8485e590f2c925c07731a9740a852
Submitter: Zuul
Branch: master

commit bb78b84c3fc8485e590f2c925c07731a9740a852
Author: Claudiu Belu <email address hidden>
Date: Wed Mar 28 01:07:18 2018 -0700

    mock: Properly patch mock.MagicMock

    MockAutospecFixture should patch both internal and external usages
    of MagicMock (mock.MagicMock and mock.mock.MagicMock). However, only
    the internal one is patched properly.

    Related-Bug: #1735588

    Change-Id: Ib9709f1cf5dbed4792f5dd7c49d8f9c77f04419f

Reviewed: https://review.openstack.org/557923
Committed: https://git.openstack.org/cgit/openstack/oslotest/commit/?id=6baff0eda4edbd8163426a2ac774514cabf99a3a
Submitter: Zuul
Branch: master

commit 6baff0eda4edbd8163426a2ac774514cabf99a3a
Author: Claudiu Belu <email address hidden>
Date: Thu Mar 29 04:57:48 2018 -0700

    mock: Apply autospec to a mock's return_value

    When creating a mock with a class as an autospec:

    mock_foo = mock.Mock(autospec=FooClass)

    The autospec is only applied to mock_foo itself, and it will
    behave as expected. However, the autospec is not applied to its
    return_value and thus, the method validation is not enforced:

    mock_foo().lish(some_argument_which_doesnt_exist=42)

    This patch addresses this issue, and adds necessary unit tests to
    test this behaviour.

    Change-Id: Icd96beba5a32001cf33f075b801471c6e7c75898
    Related-Bug: #1735588

Reviewed: https://review.openstack.org/561581
Committed: https://git.openstack.org/cgit/openstack/oslotest/commit/?id=8241dd624f8ca481fe15a44f7d3fed10be7ae937
Submitter: Zuul
Branch: master

commit 8241dd624f8ca481fe15a44f7d3fed10be7ae937
Author: Claudiu Belu <email address hidden>
Date: Fri Apr 13 10:33:12 2018 -0700

    mock: Perform patch's autospec checks on __enter__

    Currently we're doing the autospec checks on __init__ (if the
    target is autospecable), which will require importing modules
    in some cases.

    The mock.patch's constructor is called when a test module is being
    loaded, before any set up could have been run, which can be problematic
    in some cases (e.g.: some tested modules are importing platform specific
    modules, and they couldn't have been mocked yet).

    This patch moves the autospec checks to __enter__, which is executed
    after the setUp.

    Related-Bug: #1735588

    Change-Id: I9e10b34092ad795c7f9e58596fcccf4f37856225

Change abandoned by Claudiu Belu (<email address hidden>) on branch: master
Review: https://review.openstack.org/557974

Changed in nova:
assignee: Claudiu Belu (cbelu) → Stephen Finucane (stephenfinucane)
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers