Giuseppe Vallarelli has posted comments on this change.
Change subject: Functional Tests [1/3]: Added module for dummy nics
......................................................................
Patch Set 3: Code-Review-1
(7 comments)
See the different comments (they're really minor), is pretty much good already.
....................................................
File tests/functional/Makefile.am
Line 20:
Line 21: vdsmfunctestsdir = ${vdsmtestsdir}/functional
Line 22:
Line 23: dist_vdsmfunctests_PYTHON = \
Line 24: dummy.py \
I like the idea of this external module.
Line 25: momTests.py \
Line 26: networkTests.py \
Line 27: sosPluginTests.py \
Line 28: supervdsmFuncTests.py \
....................................................
File tests/functional/dummy.py
Line 16: #
Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: import random
Line 20: from contextlib import contextmanager
nitpick - can you reverse the order of the statements (before it was first
contextlib and then random).
Line 21:
Line 22: from nose.plugins.skip import SkipTest
Line 23: from vdsm.ipwrapper import linkAdd, linkDel, addrAdd, linkSet,
IPRoute2Error
Line 24:
Line 19: import random
Line 20: from contextlib import contextmanager
Line 21:
Line 22: from nose.plugins.skip import SkipTest
Line 23: from vdsm.ipwrapper import linkAdd, linkDel, addrAdd, linkSet,
IPRoute2Error
nitpick - add blank line between nose stuff and vdsm (the usual standard lib -
third party - application specific stuff)
Line 24:
Line 25:
Line 26: def create():
Line 27: """
Line 60: except IPRoute2Error:
Line 61: raise SkipTest('Failed to set device ip')
Line 62:
Line 63:
Line 64: def setLinkUp(dummyName):
I still see setLinkUp and setLinkDown instead of simply setLinkState.
Line 65: _setLinkState(dummyName, 'up')
Line 66:
Line 67:
Line 68: def setLinkDown(dummyName):
Line 61: raise SkipTest('Failed to set device ip')
Line 62:
Line 63:
Line 64: def setLinkUp(dummyName):
Line 65: _setLinkState(dummyName, 'up')
In one of his patches Toni defined a constant for the link state
http://gerrit.ovirt.org/#/c/17491/10/lib/vdsm/netinfo.py
Due to the simple string usage in many different places.
Line 66:
Line 67:
Line 68: def setLinkDown(dummyName):
Line 69: _setLinkState(dummyName, 'down')
....................................................
File tests/functional/networkTests.py
Line 22: expandPermutations, permutations)
Line 23: from testValidation import RequireDummyMod, ValidateRunningAsRoot
Line 24:
Line 25: from utils import cleanupNet, restoreNetConfig, SUCCESS, VdsProxy
Line 26: from dummy import dummyIf
nitpick import order first from dummy and then from utils.
Line 27:
Line 28:
Line 29: NETWORK_NAME = 'test-network'
Line 30: VLAN_ID = '27'
....................................................
File tests/functional/utils.py
Line 30: SUCCESS = 0
Line 31: Qos = namedtuple('Qos', 'inbound outbound')
Line 32:
Line 33:
Line 34: ip = utils.CommandPath('ip',
Is it still used? I guess no, if so you can drop it, thanks.
Line 35: '/sbin/ip', # EL6
Line 36: '/usr/sbin/ip', # Fedora
Line 37: )
Line 38:
--
To view, visit http://gerrit.ovirt.org/18039
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c7ed4110cd8d45a3fb7a9cc2cd2dc07004e96b9
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Assaf Muller <amul...@redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Assaf Muller <amul...@redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvall...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
