Dan Kenigsberg has posted comments on this change.

Change subject: tests: Adding functional tests for networking
......................................................................


Patch Set 19: Fails; I would prefer that you didn't submit this

(5 inline comments)

....................................................
File tests/functional/Makefile.am
Line 21: vdsmfunctestsdir = ${vdsmtestsdir}/functional
Line 22: 
Line 23: dist_vdsmfunctests_PYTHON = \
Line 24:        momTests.py \
Line 25:        networkTests.py \
utils.py is missing here. Please verify your tests from the 
/usr/share/vdsm/tests directory, too.
Line 26:        sosPluginTests.py \
Line 27:        xmlrpcTests.py \
Line 28:        $(NULL)


....................................................
File tests/functional/networkTests.py
Line 36:     vdsm.save_config()
Line 37: 
Line 38: 
Line 39: @expandPermutations
Line 40: class NetworkTestShould(TestCaseBase):
"NetworkTestShould" is a surprising name. Classes should usually be named as a 
noun. Could you think of a more conventional name?
Line 41: 
Line 42:     def setUp(self):
Line 43:         self.vdsm_net = VdsProxy()
Line 44: 


Line 67:             status, msg = self.vdsm_net.addNetwork(NETWORK_NAME,
Line 68:                                                    bond=bond_name,
Line 69:                                                    nics=['whatever'],
Line 70:                                                    opts={'bridged': 
bridged})
Line 71:             self.assertEqual(status, neterrors.ERR_BAD_BONDING, msg)
on el6 I get

FAIL: testFailWithInvalidBondingName(bridged=False) 
(networkTests.NetworkTestShould)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/share/vdsm/tests/testrunner.py", line 68, in wrapper
    return f(self, *args)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 71, in 
testFailWithInvalidBondingName
    self.assertEqual(status, neterrors.ERR_BAD_BONDING, msg)
AssertionError: unknown nic: whatever

you'd better include the "status" in the msg (here and elsewhere) to help 
debugging such occasions.
Line 72: 
Line 73:     def testFailWithInvalidBridgeName(self):
Line 74:         invalid_bridge_names = ('a' * 16, 'a b', 'a\tb', 'a.b', 'a:b')
Line 75:         for bridge_name in invalid_bridge_names:


....................................................
File tests/functional/utils.py
Line 47:     Creates a dummy interface, in a fixed number of attempts (100).
Line 48:     The dummy interface created has a pseudo-random name (e.g. dummy_85
Line 49:     in the format dummy_Number). Assumes root privileges.
Line 50:     """
Line 51: 
I find the randomness hard to digest. But if we have to do it, let's not 
re-invent the wheel:

 for i in random.sample(xrange(100), 100):
   name = dummy_name = 'dummy_%s' % i
   if not ip_add_dummy():
      return name
 else:
  raise SkipTest

Or even better, get a random string on module setup, and use it throughout your 
tests:

 DUMMY_PREFIX = random.sample(string.ascii_letters, 5)

This would make it easier to find out which unittets run owns which dummy.
Line 52:     rc = -1
Line 53:     attempts = 0
Line 54:     dummy_name = None
Line 55: 


....................................................
File tests/testValidation.py
Line 107:     ValidateRunningAsRoot decoration.
Line 108:     """
Line 109:     @wraps(f)
Line 110:     def wrapper(*args, **kwargs):
Line 111:         cmd_modprobe = [modprobe.cmd, "dummy"]
The assignment can safely go under the condition.
Line 112:         if not os.path.exists('/sys/module/dummy'):
Line 113:             rc, out, err = utils.execCmd(cmd_modprobe, sudo=True)
Line 114:         return f(*args, **kwargs)
Line 115:     return wrapper


-- 
To view, visit http://gerrit.ovirt.org/14840
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3be71db9dc0b92c443b87e22fe06f920055c4d3
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Giuseppe Vallarelli <gvall...@redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvall...@redhat.com>
Gerrit-Reviewer: Livnat Peer <lp...@redhat.com>
Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshz...@linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to