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: