Change in vdsm[master]: net test: Replacing MonkeyPatch with mock.patch for netinfo_...

Edward Haas has uploaded a new change for review.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_tests
......................................................................

net test: Replacing MonkeyPatch with mock.patch for netinfo_tests

Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Signed-off-by: Edward Haas <edwardh(a)redhat.com&gt;
---
M tests/network/netinfo_test.py
1 file changed, 92 insertions(+), 98 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/69/61269/1

diff --git a/tests/network/netinfo_test.py b/tests/network/netinfo_test.py
index 06dfb56..cb9e38b 100644
--- a/tests/network/netinfo_test.py
+++ b/tests/network/netinfo_test.py
@@ -26,17 +26,16 @@
 from nose.plugins.attrib import attr
 
 from vdsm.network import ipwrapper
-from vdsm.network import libvirt
-from vdsm.network import netinfo
 from vdsm.network.netinfo import addresses, bonding, dns, misc, nics, routes
 from vdsm.network.netinfo.cache import get
+from vdsm.network.netlink import addr as nl_addr
 from vdsm.utils import random_iface_name
 from vdsm import sysctl
 
 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
@@ -61,13 +60,12 @@
         with namedTemporaryDir() as temp_dir:
             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.object(dns, 'DNS_CONF_FILE', file_path):
+                for content in (RESOLV_CONF, RESOLV_CONF + '\n'):
                     with open(file_path, 'w') as file_object:
                         file_object.write(content)
 
-                    self.assertEqual(
-                        dns.get_host_nameservers(), dnss)
+                    self.assertEqual(dns.get_host_nameservers(), dnss)
 
     def testNetmaskConversions(self):
         path = os.path.join(os.path.dirname(__file__), "netmaskconversions")
@@ -91,7 +89,9 @@
             self.assertFalse(s < 0)
             self.assertTrue(s in ETHTOOL_SPEEDS or s == 0)
 
-    def testValidNicSpeed(self):
+    @mock.patch.object(nics, 'operstate')
+    @mock.patch.object(nics.io, 'open')
+    def testValidNicSpeed(self, mock_io_open, mock_operstate):
         values = ((0,           nics.OPERSTATE_UP, 0),
                   (-10,         nics.OPERSTATE_UP, 0),
                   (2 ** 16 - 1, nics.OPERSTATE_UP, 0),
@@ -102,22 +102,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)]):
-                self.assertEqual(nics.speed('fake_nic'), expected)
+            mock_io_open.return_value = io.BytesIO(str(passed))
+            mock_operstate.return_value = operstate
 
-    @MonkeyPatch(libvirt, 'networks', lambda: {'fake':
{'bridged': True}})
+            self.assertEqual(nics.speed('fake_nic'), expected)
+
+    @mock.patch('vdsm.network.netinfo.cache.libvirt.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(libvirt, 'networks', lambda: {})
-    def testGetEmpty(self):
+    @mock.patch('vdsm.network.netinfo.cache.getLinks')
+    @mock.patch('vdsm.network.netinfo.cache.libvirt.networks')
+    def testGetEmpty(self, mock_networks, mock_getLinks):
         result = {}
         result.update(get())
         self.assertEqual(result['networks'], {})
@@ -131,95 +131,88 @@
                          addresses.IPv4toMapped('127.0.0.1'))
 
     def testGetDeviceByIP(self):
-        NL_ADDRESS4 = {'label': 'iface0',
-                       'address': '127.0.0.1/32',
-                       'family': 'inet'}
-        NL_ADDRESS6 = {'label': 'iface1',
-                       'address': '2001::1:1:1/48',
-                       'family': 'inet6'}
-        NL_ADDRESSES = [NL_ADDRESS4, NL_ADDRESS6]
-
-        with MonkeyPatchScope([
-                (addresses.nl_addr, 'iter_addrs', lambda: NL_ADDRESSES)]):
-            for nl_addr in NL_ADDRESSES:
+        for addr in nl_addr.iter_addrs():
+            # Link-local IPv6 addresses are generated from the MAC address,
+            # which is shared between a nic and its bridge. Since We don't
+            # support having the same IP address on two different NICs, and
+            # link-local IPv6 addresses aren't interesting for
'getDeviceByIP'
+            # then ignore them in the test
+            if addr['scope'] != 'link':
                 self.assertEqual(
-                    nl_addr['label'],
-                   
addresses.getDeviceByIP(nl_addr['address'].split('/')[0]))
+                    addr['label'],
+                   
addresses.getDeviceByIP(addr['address'].split('/')[0]))
 
-    def _testNics(self):
-        """Creates a test fixture so that nics() reports:
-        physical nics: em, me, me0, me1, hid0 and hideous
-        dummies: fake and fake0
-        bonds: jbond (over me0 and me1)"""
-        return [ipwrapper.Link(address='f0:de:f1:da:aa:e7', index=2,
-                               linkType=ipwrapper.LinkType.NIC, mtu=1500,
-                               name='em', qdisc='pfifo_fast',
state='up'),
-                ipwrapper.Link(address='ff:de:f1:da:aa:e7', index=3,
-                               linkType=ipwrapper.LinkType.NIC, mtu=1500,
-                               name='me', qdisc='pfifo_fast',
state='up'),
-                ipwrapper.Link(address='ff:de:fa:da:aa:e7', index=4,
-                               linkType=ipwrapper.LinkType.NIC, mtu=1500,
-                               name='hid0', qdisc='pfifo_fast',
state='up'),
-                ipwrapper.Link(address='ff:de:11:da:aa:e7', index=5,
-                               linkType=ipwrapper.LinkType.NIC, mtu=1500,
-                               name='hideous', qdisc='pfifo_fast',
state='up'),
-                ipwrapper.Link(address='66:de:f1:da:aa:e7', index=6,
-                               linkType=ipwrapper.LinkType.NIC, mtu=1500,
-                               name='me0', qdisc='pfifo_fast',
state='up',
-                               master='jbond'),
-                ipwrapper.Link(address='66:de:f1:da:aa:e7', index=7,
-                               linkType=ipwrapper.LinkType.NIC, mtu=1500,
-                               name='me1', qdisc='pfifo_fast',
state='up',
-                               master='jbond'),
-                ipwrapper.Link(address='ff:aa:f1:da:aa:e7', index=34,
-                               linkType=ipwrapper.LinkType.DUMMY, mtu=1500,
-                               name='fake0', qdisc='pfifo_fast',
state='up'),
-                ipwrapper.Link(address='ff:aa:f1:da:bb:e7', index=35,
-                               linkType=ipwrapper.LinkType.DUMMY, mtu=1500,
-                               name='fake', qdisc='pfifo_fast',
state='up'),
-                ipwrapper.Link(address='66:de:f1:da:aa:e7', index=419,
-                               linkType=ipwrapper.LinkType.BOND, mtu=1500,
-                               name='jbond', qdisc='pfifo_fast',
state='up')]
-
-    def testNics(self):
+    @mock.patch.object(ipwrapper.Link, '_hiddenNics', ['hid*'])
+    @mock.patch.object(ipwrapper.Link, '_hiddenBonds', ['jb*'])
+    @mock.patch.object(ipwrapper.Link, '_fakeNics', ['fake*'])
+    @mock.patch.object(ipwrapper.Link, '_detectType',
+                       partial(_fakeTypeDetection, ipwrapper.Link))
+    @mock.patch.object(ipwrapper, '_bondExists', lambda x: x == 'jbond')
+    @mock.patch.object(misc, 'getLinks')
+    def testNics(self, mock_getLinks):
         """
         managed by vdsm: em, me, fake0, fake1
         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*'])
-                               ]):
-            self.assertEqual(set(nics.nics()),
-                             set(['em', 'me', 'fake',
'fake0']))
+
+        """Creates a test fixture so that nics() reports:
+        physical nics: em, me, me0, me1, hid0 and hideous
+        dummies: fake and fake0
+        bonds: jbond (over me0 and me1)"""
+        mock_getLinks.return_value = [
+            ipwrapper.Link(address='f0:de:f1:da:aa:e7', index=2,
+                           linkType=ipwrapper.LinkType.NIC, mtu=1500,
+                           name='em', qdisc='pfifo_fast',
state='up'),
+            ipwrapper.Link(address='ff:de:f1:da:aa:e7', index=3,
+                           linkType=ipwrapper.LinkType.NIC, mtu=1500,
+                           name='me', qdisc='pfifo_fast',
state='up'),
+            ipwrapper.Link(address='ff:de:fa:da:aa:e7', index=4,
+                           linkType=ipwrapper.LinkType.NIC, mtu=1500,
+                           name='hid0', qdisc='pfifo_fast',
state='up'),
+            ipwrapper.Link(address='ff:de:11:da:aa:e7', index=5,
+                           linkType=ipwrapper.LinkType.NIC, mtu=1500,
+                           name='hideous', qdisc='pfifo_fast',
state='up'),
+            ipwrapper.Link(address='66:de:f1:da:aa:e7', index=6,
+                           linkType=ipwrapper.LinkType.NIC, mtu=1500,
+                           name='me0', qdisc='pfifo_fast',
state='up',
+                           master='jbond'),
+            ipwrapper.Link(address='66:de:f1:da:aa:e7', index=7,
+                           linkType=ipwrapper.LinkType.NIC, mtu=1500,
+                           name='me1', qdisc='pfifo_fast',
state='up',
+                           master='jbond'),
+            ipwrapper.Link(address='ff:aa:f1:da:aa:e7', index=34,
+                           linkType=ipwrapper.LinkType.DUMMY, mtu=1500,
+                           name='fake0', qdisc='pfifo_fast',
state='up'),
+            ipwrapper.Link(address='ff:aa:f1:da:bb:e7', index=35,
+                           linkType=ipwrapper.LinkType.DUMMY, mtu=1500,
+                           name='fake', qdisc='pfifo_fast',
state='up'),
+            ipwrapper.Link(address='66:de:f1:da:aa:e7', index=419,
+                           linkType=ipwrapper.LinkType.BOND, mtu=1500,
+                           name='jbond', qdisc='pfifo_fast',
state='up')
+        ]
+
+        self.assertEqual(set(nics.nics()), set(['em', 'me',
'fake', 'fake0']))
 
     @attr(type='integration')
     @ValidateRunningAsRoot
+    @mock.patch.object(ipwrapper.Link, '_fakeNics', ['veth_*',
'dummy_*'])
     def testFakeNics(self):
-        with MonkeyPatchScope([(ipwrapper.Link, '_fakeNics', ['veth_*',
-                                                              'dummy_*'])]):
-            with veth_pair() as (v1a, v1b):
-                with dummy_device() as d1:
-                    fakes = set([d1, v1a, v1b])
-                    _nics = nics.nics()
-                    self.assertTrue(fakes.issubset(_nics),
-                                    'Fake devices %s are not listed in nics '
-                                    '%s' % (fakes, _nics))
+        with veth_pair() as (v1a, v1b):
+            with dummy_device() as d1:
+                fakes = set([d1, v1a, v1b])
+                _nics = nics.nics()
+                self.assertTrue(fakes.issubset(_nics),
+                                'Fake devices %s are not listed in nics '
+                                '%s' % (fakes, _nics))
 
-            with veth_pair(prefix='mehv_') as (v2a, v2b):
-                with dummy_device(prefix='mehd_') as d2:
-                    hiddens = set([d2, v2a, v2b])
-                    _nics = nics.nics()
-                    self.assertFalse(hiddens.intersection(_nics), 'Some of '
-                                     'hidden devices %s is shown in nics %s' %
-                                     (hiddens, _nics))
+        with veth_pair(prefix='mehv_') as (v2a, v2b):
+            with dummy_device(prefix='mehd_') as d2:
+                hiddens = set([d2, v2a, v2b])
+                _nics = nics.nics()
+                self.assertFalse(hiddens.intersection(_nics), 'Some of '
+                                 'hidden devices %s is shown in nics %s' %
+                                 (hiddens, _nics))
 
     def testGetIfaceCfg(self):
         deviceName = "___This_could_never_be_a_device_name___"
@@ -228,7 +221,7 @@
             ifcfgPrefix = os.path.join(tempDir, 'ifcfg-')
             filePath = ifcfgPrefix + deviceName
 
-            with MonkeyPatchScope([(misc, 'NET_CONF_PREF', ifcfgPrefix)]):
+            with mock.patch.object(misc, 'NET_CONF_PREF', ifcfgPrefix):
                 with open(filePath, 'w') as ifcfgFile:
                     ifcfgFile.write(ifcfg)
                 self.assertEqual(
@@ -237,9 +230,10 @@
                     misc.getIfaceCfg(deviceName)['NETMASK'],
'255.255.0.0')
 
     @brokentest("Skipped becasue it breaks randomly on the CI")
-    @MonkeyPatch(bonding, 'BONDING_DEFAULTS', bonding.BONDING_DEFAULTS
-                 if os.path.exists(bonding.BONDING_DEFAULTS)
-                 else '../vdsm/bonding-defaults.json')
+    @mock.patch.object(bonding, 'BONDING_DEFAULTS',
+                       bonding.BONDING_DEFAULTS
+                       if os.path.exists(bonding.BONDING_DEFAULTS)
+                       else '../vdsm/bonding-defaults.json')
     @attr(type='integration')
     @ValidateRunningAsRoot
     @RequireBondingMod


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;

gerrit-hooks has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_tests
......................................................................


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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Edward Haas has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 2: Verified+1

Passing unit tests.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Jenkins CI RO]

Jenkins CI has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 2: Continuous-Integration+1

Propagate review hook: Continuous Integration value inherited from patch 1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Petr Horáček has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 2: Code-Review-1

(6 comments)

https://gerrit.ovirt.org/#/c/61269/2/tests/network/netinfo_test.py
File tests/network/netinfo_test.py:

PS2, Line 111: lambda: {'fake': {'bridged': True}}
i think we can use return_value here. Not sure if we want to use shorter 'lambda'
or be consistent with return_value.


PS2, Line 118: vdsm.network.netinfo.cache
won't be better to import cache with others modules from netinfo and then use
patch.object?


PS2, Line 118:     @mock.patch('vdsm.network.netinfo.cache.getLinks')
             :     @mock.patch('vdsm.network.netinfo.cache.libvirt.networks')
if we do not specify return_value, mocked function returns the same and has the same
side-effect as the original one.


PS2, Line 149: partial(_fakeTypeDetection, ipwrapper.Link)
we can use lambda requiring one parameter, _fakeTypeDetections is just:

def _fakeTypeDetection(cls, devname):                                           
    pass


PS2, Line 159:         """Creates a test fixture so that nics() reports:
             :         physical nics: em, me, me0, me1, hid0 and hideous
             :         dummies: fake and fake0
             :         bonds: jbond (over me0 and me1)"""
if it is not a part of method docstring, we should write it as inline comment (with # on
each line)


PS2, Line 164:             ipwrapper.Link(address='f0:de:f1:da:aa:e7', index=2,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='em', qdisc='pfifo_fast',
state='up'),
             :             ipwrapper.Link(address='ff:de:f1:da:aa:e7', index=3,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='me', qdisc='pfifo_fast',
state='up'),
             :             ipwrapper.Link(address='ff:de:fa:da:aa:e7', index=4,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='hid0', qdisc='pfifo_fast',
state='up'),
             :             ipwrapper.Link(address='ff:de:11:da:aa:e7', index=5,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='hideous',
qdisc='pfifo_fast', state='up'),
             :             ipwrapper.Link(address='66:de:f1:da:aa:e7', index=6,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='me0', qdisc='pfifo_fast',
state='up',
             :                            master='jbond'),
             :             ipwrapper.Link(address='66:de:f1:da:aa:e7', index=7,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='me1', qdisc='pfifo_fast',
state='up',
             :                            master='jbond'),
             :             ipwrapper.Link(address='ff:aa:f1:da:aa:e7', index=34,
             :                            linkType=ipwrapper.LinkType.DUMMY, mtu=1500,
             :                            name='fake0',
qdisc='pfifo_fast', state='up'),
             :             ipwrapper.Link(address='ff:aa:f1:da:bb:e7', index=35,
             :                            linkType=ipwrapper.LinkType.DUMMY, mtu=1500,
             :                            name='fake', qdisc='pfifo_fast',
state='up'),
             :             ipwrapper.Link(address='66:de:f1:da:aa:e7', index=419,
             :                            linkType=ipwrapper.LinkType.BOND, mtu=1500,
             :                            name='jbond',
qdisc='pfifo_fast', state='up')
             :         ]
can we define it outside the method in a constant?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Edward Haas has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 2:

(6 comments)

https://gerrit.ovirt.org/#/c/61269/2/tests/network/netinfo_test.py
File tests/network/netinfo_test.py:

PS2, Line 111: lambda: {'fake': {'bridged': True}}
>  i think we can use return_value here. Not sure if we want to use
> shorter 'l 
Using return_value suggests that we need a generated mock object, but
we do not need it here, we just provide a stub.


PS2, Line 118: vdsm.network.netinfo.cache
>  won't be better to import cache with others modules from netinfo
> and then u 
Yes, but it is not a must and importing the whole module seemed
overdoing.
I can do it if you think it is worth it.


PS2, Line 118:     @mock.patch('vdsm.network.netinfo.cache.getLinks')
             :     @mock.patch('vdsm.network.netinfo.cache.libvirt.networks')
>  if we do not specify return_value, mocked function returns the same
> and has 
If it is not specified, it returns None, which is fine in this case.


PS2, Line 149: partial(_fakeTypeDetection, ipwrapper.Link)
>  we can use lambda requiring one parameter, _fakeTypeDetections is
> just: 
Ok, will clean it up in a following patch.


PS2, Line 159:         """Creates a test fixture so that nics() reports:
             :         physical nics: em, me, me0, me1, hid0 and hideous
             :         dummies: fake and fake0
             :         bonds: jbond (over me0 and me1)"""
>  if it is not a part of method docstring, we should write it as inline
> comme 
Done


PS2, Line 164:             ipwrapper.Link(address='f0:de:f1:da:aa:e7', index=2,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='em', qdisc='pfifo_fast',
state='up'),
             :             ipwrapper.Link(address='ff:de:f1:da:aa:e7', index=3,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='me', qdisc='pfifo_fast',
state='up'),
             :             ipwrapper.Link(address='ff:de:fa:da:aa:e7', index=4,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='hid0', qdisc='pfifo_fast',
state='up'),
             :             ipwrapper.Link(address='ff:de:11:da:aa:e7', index=5,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='hideous',
qdisc='pfifo_fast', state='up'),
             :             ipwrapper.Link(address='66:de:f1:da:aa:e7', index=6,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='me0', qdisc='pfifo_fast',
state='up',
             :                            master='jbond'),
             :             ipwrapper.Link(address='66:de:f1:da:aa:e7', index=7,
             :                            linkType=ipwrapper.LinkType.NIC, mtu=1500,
             :                            name='me1', qdisc='pfifo_fast',
state='up',
             :                            master='jbond'),
             :             ipwrapper.Link(address='ff:aa:f1:da:aa:e7', index=34,
             :                            linkType=ipwrapper.LinkType.DUMMY, mtu=1500,
             :                            name='fake0',
qdisc='pfifo_fast', state='up'),
             :             ipwrapper.Link(address='ff:aa:f1:da:bb:e7', index=35,
             :                            linkType=ipwrapper.LinkType.DUMMY, mtu=1500,
             :                            name='fake', qdisc='pfifo_fast',
state='up'),
             :             ipwrapper.Link(address='66:de:f1:da:aa:e7', index=419,
             :                            linkType=ipwrapper.LinkType.BOND, mtu=1500,
             :                            name='jbond',
qdisc='pfifo_fast', state='up')
             :         ]
>  can we define it outside the method in a constant? 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Petr Horáček has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/61269/2/tests/network/netinfo_test.py
File tests/network/netinfo_test.py:

PS2, Line 118: vdsm.network.netinfo.cache
>  Yes, but it is not a must and importing the whole module seemed
> overdoing. 
Is's ok, leave it as it is.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

gerrit-hooks has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Edward Haas has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Petr Horáček has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 3:

(1 comment)

partial review

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

PS3, Line 134:         for addr in nl_addr.iter_addrs():
             :             # Link-local IPv6 addresses are generated from the MAC
address,
             :             # which is shared between a nic and its bridge. Since We
don't
             :             # support having the same IP address on two different NICs,
and
             :             # link-local IPv6 addresses aren't interesting for
'getDeviceByIP'
             :             # then ignore them in the test
             :             if addr['scope'] != 'link':
             :                 self.assertEqual(
             :                     addr['label'],
             :                    
addresses.getDeviceByIP(addr['address'].split('/')[0]))
this seems unrelated to mock introduction. I think that this test should not depend on the
host having interfaces.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 3: Code-Review-1

(1 comment)

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

PS3, Line 134:         for addr in nl_addr.iter_addrs():
             :             # Link-local IPv6 addresses are generated from the MAC
address,
             :             # which is shared between a nic and its bridge. Since We
don't
             :             # support having the same IP address on two different NICs,
and
             :             # link-local IPv6 addresses aren't interesting for
'getDeviceByIP'
             :             # then ignore them in the test
             :             if addr['scope'] != 'link':
             :                 self.assertEqual(
             :                     addr['label'],
             :                    
addresses.getDeviceByIP(addr['address'].split('/')[0]))
>  this seems unrelated to mock introduction. I think that this test
> should no 
hmm, this basically reverts your https://gerrit.ovirt.org/59881 . Please
keep this a pure (albeit quite stupid) unit test.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Edward Haas has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 3: -Verified

(1 comment)

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

PS3, Line 134:         for addr in nl_addr.iter_addrs():
             :             # Link-local IPv6 addresses are generated from the MAC
address,
             :             # which is shared between a nic and its bridge. Since We
don't
             :             # support having the same IP address on two different NICs,
and
             :             # link-local IPv6 addresses aren't interesting for
'getDeviceByIP'
             :             # then ignore them in the test
             :             if addr['scope'] != 'link':
             :                 self.assertEqual(
             :                     addr['label'],
             :                    
addresses.getDeviceByIP(addr['address'].split('/')[0]))
>  hmm, this basically reverts your https://gerrit.ovirt.org/59881 .
> Please ke 
So this was a bad rebase? ahh
Will fix, thanks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

gerrit-hooks has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Edward Haas has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Edward Haas has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 5: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


net test: Replacing MonkeyPatch with mock.patch for netinfo_test

Refactor netinfo_test a bit and use mock module.

Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Signed-off-by: Edward Haas <edwardh(a)redhat.com&gt;
Reviewed-on: https://gerrit.ovirt.org/61269
Continuous-Integration: Jenkins CI
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M tests/network/netinfo_test.py
1 file changed, 85 insertions(+), 87 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Dan Kenigsberg: Looks good to me, approved
  Edward Haas: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;

gerrit-hooks has posted comments on this change.

Change subject: net test: Replacing MonkeyPatch with mock.patch for netinfo_test
......................................................................


Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4e14162bb2773c50e551f4409c4c37cffe9f2a3
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No