Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example

edwardh at redhat.com edwardh at redhat.com
Wed Apr 6 08:35:31 UTC 2016


Edward Haas has posted comments on this change.

Change subject: test: Replacing MonkeyPatch with mock.patch example
......................................................................


Patch Set 3:

(3 comments)

The added value part has been demonstrated in the previous patch, I intentionally preserved the minimal functionality to show how the same is implemented with mock, without any special improvement.

https://gerrit.ovirt.org/#/c/55603/3/tests/network/netinfo_test.py
File tests/network/netinfo_test.py:

Line 114
Line 115
Line 116
Line 117
Line 118
> Please remove the unneeded monkeypatching in separate patch, this is not re
The conversion from mockeypatch to mock includes mockeypatch to None.
Nothing will happen if this will not get in quickly, its a nice to have harmless thing.


Line 115:         # it should.
Line 116:         get()
Line 117: 
Line 118:     @mock.patch('vdsm.netinfo.cache.getLinks', lambda: [])
Line 119:     @mock.patch('vdsm.netinfo.cache.netinfo.networks', lambda: {})
> Mocking strings instead of modules is not an improvement.
We probably need to sync the expectations first: This patch was not intended to show why mock is better, just to show that it can do the same things that we have already done so far.

In this case, there is an improvement in the test module dependency: It does not need to import the code under test dependencies, it handles it for you.
Line 120:     def testGetEmpty(self):
Line 121:         result = {}
Line 122:         result.update(get())
Line 123:         self.assertEqual(result['networks'], {})


Line 189:                 mock.patch('vdsm.ipwrapper.Link._detectType',
Line 190:                            partial(_fakeTypeDetection, ipwrapper.Link)), \
Line 191:                 mock.patch('vdsm.ipwrapper.Link._fakeNics', ['fake*']), \
Line 192:                 mock.patch('vdsm.ipwrapper.Link._hiddenBonds', ['jb*']), \
Line 193:                 mock.patch('vdsm.ipwrapper.Link._hiddenNics', ['hid*']):
> This huge block is super ugly, not an improvement over old code.
Sure it's ugly, not because of the test code or the patching, but because of the production test that requires such mocking.

Both versions seem ugly to me.
I'm not sure what the last comment line is all about, the whole point is for everything to be reverted back after the context is over, that's why it is used as a decorator or contextmanager.
Line 194:             self.assertEqual(set(nics.nics()),
Line 195:                              set(['em', 'me', 'fake', 'fake0']))
Line 196: 
Line 197:     @attr(type='integration')


-- 
To view, visit https://gerrit.ovirt.org/55603
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I34ef99c00e7e2e4bbf13a52ec8471815e81d2a9e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list