<div dir="ltr"><div>Hello all,</div><div><br></div><div>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. </div>
<div><br></div><div>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. </div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div><div><br></div><div><div>~ ktbenton$ python</div>
<div>Python 2.7.5 (default, Aug 25 2013, 00:04:04)</div><div>[GCC 4.2.1 Compatible Apple LLVM 5.0 (clang-500.0.68)] on darwin</div><div>Type "help", "copyright", "credits" or "license" for more information.</div>
<div>>>> import mock</div><div>>>> import random as r1</div><div>>>> p1 = mock.patch.object(r1, 'random').start()</div><div>>>> p2 = mock.patch.object(r1, 'random').start()</div>
<div>>>> r1.random()</div><div><MagicMock name='random()' id='<a href="tel:4372335696" value="+14372335696" target="_blank">4372335696</a>'></div><div>>>> mock.patch.stopall()</div>
<div>>>> r1.random()</div><div><MagicMock name='random()' id='<a href="tel:4372330576" value="+14372330576" target="_blank">4372330576</a>'></div>
<div>>>> import random as r2</div><div>>>> r2.random()</div><div><MagicMock name='random()' id='<a href="tel:4372330576" value="+14372330576" target="_blank">4372330576</a>'></div>
<div>>>></div></div><div><br></div><div>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.</div>
<div><br></div><div><br></div><div>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.</div>
<div><br></div><div><br></div><div>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.</div><div><br></div><div>1. <a href="https://review.openstack.org/#/c/86538/" target="_blank">https://review.openstack.org/#/c/86538/</a></div>
<div>2. <a href="https://bugs.launchpad.net/neutron/+bug/1307025" target="_blank">https://bugs.launchpad.net/neutron/+bug/1307025</a></div><div>3. <a href="https://bugs.launchpad.net/neutron/+bug/1307038" target="_blank">https://bugs.launchpad.net/neutron/+bug/1307038</a></div>
<div>4. <a href="https://review.openstack.org/#/c/87097/" target="_blank">https://review.openstack.org/#/c/87097/</a></div><div>5. <a href="https://code.google.com/p/mock/source/browse/mock.py#1133" target="_blank">https://code.google.com/p/mock/source/browse/mock.py#1133</a></div>
<div>6. <a href="https://code.google.com/p/mock/source/browse/mock.py#1719" target="_blank">https://code.google.com/p/mock/source/browse/mock.py#1719</a></div><div>7. <a href="https://code.google.com/p/mock/issues/detail?id=226" target="_blank">https://code.google.com/p/mock/issues/detail?id=226</a></div>
<div><br></div><div><br></div>-- <br><div>Kevin Benton</div>
</div>