Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
gerrit-hooks has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 5: * Update tracker: IGNORE, no Bug-Url found -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Edward Haas has abandoned this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Abandoned -- To view, visit https://gerrit.ovirt.org/55603 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I34ef99c00e7e2e4bbf13a52ec8471815e81d2a9e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
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
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Petr Horáček
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
gerrit-hooks has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 5: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
gerrit-hooks has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 4: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Nir Soffer has posted comments on this change.
Change subject: test: Replacing MonkeyPatch with mock.patch example
..
Patch Set 3:
(2 comments)
https://gerrit.ovirt.org/#/c/55603/3/tests/network/netinfo_test.py
File tests/network/netinfo_test.py:
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',
> we do prefer to use the standard mock. i don't see any issue with this appr
Using standard modules instead of our own is good idea in general, but:
- We have lot of code using our monkeypatch module
- We don't have time to convert old code to mock, and converting has no
significant benefit
- So we are going to have both monkeypatch and mock
- And when we move to pytest, we want to use pytest monkeypatch, which is much
better then our monkeypatch or mock (see the example in my other comment).
We should instead spend the time on moving to pytest.
We can add the mock module for mocking, eliminating simple fake objects, but
for monkeypatching we should keep the current code.
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: {})
> We probably need to sync the expectations first: This patch was not intende
I don't like this magic, importing stuff behind by back.
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
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Petr Horáček
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Yaniv Bronhaim has posted comments on this change.
Change subject: test: Replacing MonkeyPatch with mock.patch example
..
Patch Set 3:
(2 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
> The conversion from mockeypatch to mock includes mockeypatch to None.
you can separate it to another patch with explanation why it is not needed any
more in the commit msg. more appropriate
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',
> The original code was better. Having a way to monkey patch multiple modules
we do prefer to use the standard mock. i don't see any issue with this
approach, its not uglier not prettier than using MonkeyPatch and it doesn't
matter. the idea here is to use well documented and standard python mock library
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',
--
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
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Petr Horáček
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Edward Haas has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 3: Again, this patch is not intended to convince or present any advantages of mock over mockeypatch. This patch is intended to show that it does not cause any lose of functionality from what currently exist (if one wants to replace monkeypatch with mock). Please review https://gerrit.ovirt.org/#/c/55342 to see what can be done with mock that cannot be done with monkeypatch. -- 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 Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Nir Soffer has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 3: This change is not a reason to use mock module for patching. This change that does not buy us anything now and will require additional work when we move to py.test. Better to keep the current code and remove it when we move to py.test. -- 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 Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
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
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Petr Horáček
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Nir Soffer has posted comments on this change.
Change subject: test: Replacing MonkeyPatch with mock.patch example
..
Patch Set 3: Code-Review-1
(9 comments)
I'm not convinced that using mock adds any value here, using py.test
monkeypatch is much cleaner way to monkeypatch.
Please separate the unrelated changes to another patch (not on top of this).
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
related to using mock, and can be merge quickly now.
Line 272
Line 273
Line 274
Line 275
Line 276
Please remove this in another patch.
Line 282
Line 283
Line 284
Line 285
Line 286
Please remove this in another patch.
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',
The original code was better. Having a way to monkey patch multiple modules is
better than multiple or nested with.
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: {})
Mocking strings instead of modules is not an improvement.
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.
With pytest we can this much nicers like this:
def test_foo(monkeypatch):
monkeypatch.setattr(module1, "foo", fake_foo)
monkeypatch.setattr(module2, "bar", fake_bar)
monkeypatch.setattr(module3, "baz", fake_baz)
All the monkeypatching is automaticaly and safely reverted when the test ends.
Line 194: self.assertEqual(set(nics.nics()),
Line 195: set(['em', 'me', 'fake', 'fake0']))
Line 196:
Line 197: @attr(type='integration')
Line 197: @attr(type='integration')
Line 198: @ValidateRunningAsRoot
Line 199: def testFakeNics(self):
Line 200: with mock.patch('vdsm.ipwrapper.Link._fakeNics', ['veth_*',
Line 201: 'dummy_*']):
No improvement here.
Line 202: with veth_pair() as (v1a, v1b):
Line 203: with dummy_device() as d1:
Line 204: fakes = set([d1, v1a, v1b])
Line 205: _nics = nics.nics()
Line 221: with namedTemporaryDir() as tempDir:
Line 222: ifcfgPrefix = os.path.join(tempDir, 'ifcfg-')
Line 223: filePath = ifcfgPrefix + deviceName
Line 224:
Line 225: with mock.patch('vdsm.netinfo.misc.NET_CONF_PREF',
ifcfgPrefix):
Same
Line 226: with open(filePath, 'w') as ifcfgFile:
Line 227: ifcfgFile.write(ifcfg)
Line 228: self.assertEqual(
Line 229: misc.getIfaceCfg(deviceName)['GATEWAY'],
'1.1.1.1')
Line 233: @brokentest("Skipped becasue it breaks randomly on the CI")
Line 234: @mock.patch('vdsm.netinfo.bonding.BONDING_DEFAULTS',
Line 235: bonding.BONDING_DEFAULTS
Line 236: if os.path.exists(bonding.BONDING_DEFAULTS)
Line 237: else '../vdsm/bonding-defaults.json')
Simila, I prefer the version using the module instead of the magical string
version.
Line 238: @attr(type='integration')
Line 239: @ValidateRunningAsRoot
Line 240: @RequireBondingMod
Line 241: def testGetBondingOptions(self):
--
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
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Petr Horáček
Gerrit-Reviewer: Piotr Kliczew
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
gerrit-hooks has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 3: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- 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 Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Piotr Kliczewski has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 2: Code-Review+1 -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Petr Horáček has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 2: Code-Review+1 -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Yaniv Bronhaim has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 2: Code-Review+1 -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Edward Haas has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 2: Verified+1 -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Edward Haas has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 2: (3 comments) https://gerrit.ovirt.org/#/c/55603/2/tests/network/netinfo_test.py File tests/network/netinfo_test.py: Line 87 Line 88 Line 89 Line 90 Line 91 > what about those mocks ^? It was converted to None :) These are mocks that are no longer needed. These are good examples of why a monkey patch is not a good choice for some mocking: At some point a mock was needed, after several code changes, the dependency that required the mock has been dropped (or changed).. However, the mock has not been taken out, as no error has hinted that it should. To avoid such cases, when a mock is used, we should check it was called/used. Line 114 Line 115 Line 116 Line 117 Line 118 > what about _detectType and _GetBondingOptions mocks? Same as the previous. Line 282 Line 283 Line 284 Line 285 Line 286 > how is it not needed anymore? Dan helped me figure this out, I had trouble understanding it myself (why it passes without the mocking). In testrunner.py we assign: constants.P_VDSM = "../vdsm/" for local runs. -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Yaniv Bronhaim has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 2: (3 comments) https://gerrit.ovirt.org/#/c/55603/2/tests/network/netinfo_test.py File tests/network/netinfo_test.py: Line 87 Line 88 Line 89 Line 90 Line 91 what about those mocks ^? Line 114 Line 115 Line 116 Line 117 Line 118 what about _detectType and _GetBondingOptions mocks? Line 282 Line 283 Line 284 Line 285 Line 286 how is it not needed anymore? -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Edward Haas has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 2: This patch demonstrates how mock can be used to replace MonkeyPatch. However, I highly recommend not to use it in this manner (for new code) unless you have to. I would recommend using the automatic mockers that mock provides, setting them up and then asserting on them. -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Petr Horáček Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
gerrit-hooks has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 2: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Edward Haas has posted comments on this change.
Change subject: test: Replacing MonkeyPatch with mock.patch example
..
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/55603/1/tests/network/netinfo_test.py
File tests/network/netinfo_test.py:
Line 261:
Line 262: finally:
Line 263: bonds.write('-' + bondName)
Line 264:
Line 265: # @MonkeyPatch(bonding, 'BONDING_NAME2NUMERIC_PATH',
The tests pass without these patches.
However, I do not fully understand why the patching is not needed, so until I
clarify the 'why', I'm commenting the patch.
Line 266: # bonding.BONDING_NAME2NUMERIC_PATH
Line 267: # if
os.path.exists(bonding.BONDING_NAME2NUMERIC_PATH)
Line 268: # else '../vdsm/bonding-name2numeric.json')
Line 269: def test_get_bonding_option_numeric_val_exists(self):
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
Edward Haas has uploaded a new change for review.
Change subject: test: Replacing MonkeyPatch with mock.patch example
..
test: Replacing MonkeyPatch with mock.patch example
Replacing MonkeyPatch with mock.patch in netinfo tests, presenting how
to implement the same patching with mock.patch.
Note: The patching logic has been preserved but some patches have been
removed due to no use.
Change-Id: I34ef99c00e7e2e4bbf13a52ec8471815e81d2a9e
Signed-off-by: Edward Haas
---
M tests/network/netinfo_test.py
1 file changed, 33 insertions(+), 41 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/03/55603/1
diff --git a/tests/network/netinfo_test.py b/tests/network/netinfo_test.py
index c8395dd..311555e 100644
--- a/tests/network/netinfo_test.py
+++ b/tests/network/netinfo_test.py
@@ -26,7 +26,6 @@
from nose.plugins.attrib import attr
from vdsm import ipwrapper
-from vdsm import netinfo
from vdsm.netinfo import addresses, bonding, dns, misc, nics, routes
from vdsm.netinfo.cache import get
from vdsm.netlink import addr as nl_addr
@@ -35,8 +34,8 @@
from .ipwrapper_test import _fakeTypeDetection
from modprobe import RequireBondingMod
-from monkeypatch import MonkeyPatch, MonkeyPatchScope
from .nettestlib import dnsmasq_run, dummy_device, veth_pair, wait_for_ipv6
+from testlib import mock
from testlib import VdsmTestCase as TestCaseBase, namedTemporaryDir
from testValidation import ValidateRunningAsRoot
from testValidation import brokentest
@@ -62,7 +61,7 @@
file_path = os.path.join(temp_dir, 'resolv.conf')
for content in (RESOLV_CONF, RESOLV_CONF + '\n'):
-with MonkeyPatchScope([(dns, 'DNS_CONF_FILE', file_path)]):
+with mock.patch('vdsm.netinfo.dns.DNS_CONF_FILE', file_path):
with open(file_path, 'w') as file_object:
file_object.write(content)
@@ -81,14 +80,10 @@
self.assertRaises(ValueError, addresses.prefix2netmask, -1)
self.assertRaises(ValueError, addresses.prefix2netmask, 33)
-@MonkeyPatch(ipwrapper.Link, '_detectType',
- partial(_fakeTypeDetection, ipwrapper.Link))
def testSpeedInvalidNic(self):
nicName = '0' * 20 # devices can't have so long names
self.assertEqual(nics.speed(nicName), 0)
-@MonkeyPatch(ipwrapper.Link, '_detectType',
- partial(_fakeTypeDetection, ipwrapper.Link))
def testSpeedInRange(self):
for d in nics.nics():
s = nics.speed(d)
@@ -106,24 +101,22 @@
(123, 'unknown',0))
for passed, operstate, expected in values:
-with MonkeyPatchScope([(io, 'open',
-lambda x: io.BytesIO(str(passed))),
- (nics, 'operstate',
-lambda x: operstate)]):
+with mock.patch('vdsm.netinfo.nics.io.open',
+lambda x: io.BytesIO(str(passed))), \
+mock.patch('vdsm.netinfo.nics.operstate',
+ lambda x: operstate):
self.assertEqual(nics.speed('fake_nic'), expected)
-@MonkeyPatch(ipwrapper.Link, '_detectType',
- partial(_fakeTypeDetection, ipwrapper.Link))
-@MonkeyPatch(netinfo, 'networks', lambda: {'fake': {'bridged': True}})
-@MonkeyPatch(bonding, '_getBondingOptions', lambda x: {})
+@mock.patch('vdsm.netinfo.cache.netinfo.networks',
+lambda: {'fake': {'bridged': True}})
def testGetNonExistantBridgeInfo(self):
# Getting info of non existing bridge should not raise an exception,
# just log a traceback. If it raises an exception the test will fail as
# it should.
get()
-@MonkeyPatch(netinfo.cache, 'getLinks', lambda: [])
-@MonkeyPatch(netinfo, 'networks', lambda: {})
+@mock.patch('vdsm.netinfo.cache.getLinks', lambda: [])
+@mock.patch('vdsm.netinfo.cache.netinfo.networks', lambda: {})
def testGetEmpty(self):
result = {}
result.update(get())
@@ -190,24 +183,22 @@
not managed due to hidden bond (jbond) enslavement: me0, me1
not managed due to being hidden nics: hid0, hideous
"""
-with MonkeyPatchScope([(netinfo.misc, 'getLinks',
-self._testNics),
- (ipwrapper, '_bondExists',
-lambda x: x == 'jbond'),
- (ipwrapper.Link, '_detectType',
-partial(_fakeTypeDetection, ipwrapper.Link)),
- (ipwrapper.Link, '_fakeNics', ['fake*']),
- (ipwrapper.Link, '_hiddenBonds', ['jb*']),
- (ipwrapper.Link, '_hiddenNics', ['hid*'])
- ])
Change in vdsm[master]: test: Replacing MonkeyPatch with mock.patch example
gerrit-hooks has posted comments on this change. Change subject: test: Replacing MonkeyPatch with mock.patch example .. Patch Set 1: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
