Dan Kenigsberg has posted comments on this change.
Change subject: Functional Tests [2/3]: Added ipwrapper.ruleExists+routeExists
......................................................................
Patch Set 5: Code-Review-1
(6 comments)
....................................................
File lib/vdsm/ipwrapper.py
Line 38:
Line 39: def equals(cls):
Line 40: def __eq__(self, other):
Line 41: return self.__dict__ == other.__dict__
Line 42: cls.__eq__ = __eq__
For completeness, you should make sure that type(other) is cls.
Line 43: return cls
Line 44:
Line 45:
Line 46: @equals
Line 138: self.detached = detached
Line 139:
Line 140: @classmethod
Line 141: def parse(cls, text):
Line 142: detached = '[detached]' in text
please provide a unit test with [detached] in text. Frankly, I fear wild
in-text searches. They end up finding the text in unexpectedly wrong places.
Line 143: rule = [entry for entry in text.split() if entry !=
'[detached]']
Line 144: parameters = rule[1:]
Line 145:
Line 146: if len(rule) % 2 == 0:
Line 182: source = None
Line 183: if destination == 'all':
Line 184: destination = None
Line 185: srcDevice = data.get('dev') or data.get('iif')
Line 186: detached = data.get('detached')
do we want to have detached that is None?
Line 187:
Line 188: return cls(table, source=source, destination=destination,
Line 189: srcDevice=srcDevice, detached=detached)
Line 190:
Line 248: command += route
Line 249: _execCmd(command)
Line 250:
Line 251:
Line 252: def _getValidEntries(constructor, list):
One should not override a basic type such as "list". Particularly if "list" may
be any other iterable, too.
Line 253: for entry in list:
Line 254: try:
Line 255: yield constructor(entry)
Line 256: except ValueError:
....................................................
File tests/functional/utils.py
Line 70:
Line 71: @wraps(func)
Line 72: def wrapper(*args, **kwargs):
Line 73: try:
Line 74: # Save the rules before the test
redundant comment, imho.
Line 75: base = ipwrapper.ruleList()
Line 76:
Line 77: func(*args, **kwargs)
Line 78: except Exception:
Line 83:
Line 84:
Line 85: def restoreRules(base):
Line 86: current = ipwrapper.ruleList()
Line 87: added = list(set(current) - set(base))
there is no need to convert the set into a list.
Line 88: for rule in added:
Line 89: ipwrapper.ruleDel(ipwrapper.Rule.fromText(rule))
Line 90:
Line 91:
--
To view, visit http://gerrit.ovirt.org/18040
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf7c28e85f53f51879750308f3a9f93fc635cf33
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Assaf Muller <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Assaf Muller <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
