Open Stack

Hello all,

A recent patch merged to remove all of the self.addCleanup(patch.stop)
calls since we now have mock.patch.stopall in the cleanup of the base test
case to do that work[1]. This introduced a couple of nondeterministic bugs
that allowed it to sneak through the gate and block future patches [2][3].
The two bugs were subsequently fixed in another patch[4] so the gate should
be okay now. However, the root cause of this is something reviewers will
have to watch for.

The problem stems from the fact that mock.patch stores the active patches
in a set [5]. Sets do not preserve ordering so when the set is converted to
a list during stopall [6], it will stop patches in an arbitrary order each
time it's run.

This becomes a problem is when there are multiple patches to the same
target (e.g. one in a setUp method and another in a specific test which
requires different behavior from the patch). Patch 1 will keep a reference
to the original target and replace it with its substitution. Patch 2 will
do the same, but it will record the patch 1's substitution as the original.
If the patches stop in the reverse order that they were started, everything
will be okay. But if they stop in the same order, patch 1 will stop first
and replace the target with the original. Then patch 2 will stop and
replace the target with patch 1's  substitution since that's what patch2
had recorded as the original target. This leaves a substitution after
stopall has been called, which can affect subsequent unit tests.

The following is an interactive example you can run to see this behavior.
However, you may get lucky on the set->list conversion and get the expected
behavior.

~ ktbenton$ python
Python 2.7.5 (default, Aug 25 2013, 00:04:04)
[GCC 4.2.1 Compatible Apple LLVM 5.0 (clang-500.0.68)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import mock
>>> import random as r1
>>> p1 = mock.patch.object(r1, 'random').start()
>>> p2 = mock.patch.object(r1, 'random').start()
>>> r1.random()
<MagicMock name='random()' id='4372335696'>
>>> mock.patch.stopall()
>>> r1.random()
<MagicMock name='random()' id='4372330576'>
>>> import random as r2
>>> r2.random()
<MagicMock name='random()' id='4372330576'>
>>>

In the above, calling r1.random is still returning a MagickMock object
after stopall has been called. Specifically, it is returning the mock from
the first patch (note the difference in the MagicMock ids when calling
r1.random before and after stopall). The last two commands illustrate how
this can affect unrelated tests by carrying through to future imports.


In summary, do not allow tests to run multiple patches on the same target
without explicitly stopping the latter patches before the first one.
Putting patch calls in 'with' blocks automatically solves this, so use them
when possible.


There is also a possible fix to the mock library for this [7], but I find
it unlikely for this behavior to be changed anytime soon.

1. https://review.openstack.org/#/c/86538/
2. https://bugs.launchpad.net/neutron/+bug/1307025
3. https://bugs.launchpad.net/neutron/+bug/1307038
4. https://review.openstack.org/#/c/87097/
5. https://code.google.com/p/mock/source/browse/mock.py#1133
6. https://code.google.com/p/mock/source/browse/mock.py#1719
7. https://code.google.com/p/mock/issues/detail?id=226


-- 
Kevin Benton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140413/3a39c9d4/attachment.html>