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

edwardh at redhat.com edwardh at redhat.com
Sun Apr 10 06:42:41 UTC 2016


Edward Haas has posted comments on this change.

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


Patch Set 3:

(3 comments)

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
> you can separate it to another patch with explanation why it is not needed 
Ok guys, splitting this one.


Line 102: 
Line 103:         for passed, operstate, expected in values:
Line 104:             with mock.patch('vdsm.netinfo.nics.io.open',
Line 105:                             lambda x: io.BytesIO(str(passed))), \
Line 106:                     mock.patch('vdsm.netinfo.nics.operstate',
> Using standard modules instead of our own is good idea in general, but:
I tried to prettify this in a following patch.

I don't think anyone raised the requirement to get rid of the existing monkey patches or to avoid py.test ones.
I do think that adding a dependency on a specific test runner is not a good choice if there are other options available, for the same reasons we want to move to py.test: In the future, a better runner may pop up.

mock provides a full mock tooling that covers monkey patches and provides much more capabilities (that I needed and used in the previous patch).
Line 107:                                lambda x: operstate):
Line 108:                 self.assertEqual(nics.speed('fake_nic'), expected)
Line 109: 
Line 110:     @mock.patch('vdsm.netinfo.cache.netinfo.networks',


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: {})
> I don't like this magic, importing stuff behind by back.
Well, I did not like the fact that I need to import into the test something that the test itself is not using.
Explicitly importing in order to mock its members is noise IMO.

But I guess this is a taste thing and probably influenced by our experience in the past.
Line 120:     def testGetEmpty(self):
Line 121:         result = {}
Line 122:         result.update(get())
Line 123:         self.assertEqual(result['networks'], {})


-- 
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