Ido Barkan has posted comments on this change. Change subject: netlink: event monitor ......................................................................
Patch Set 27: Code-Review-1 (6 comments) http://gerrit.ovirt.org/#/c/36197/27/lib/vdsm/netlink/monitor.py File lib/vdsm/netlink/monitor.py: Line 75: ipv4-route ipv6-ifaddr, ipv6-mroute, ipv6-route, ipv6-ifinfo, Line 76: decnet-ifaddr, decnet-route, ipv6-prefix Line 77: """ Line 78: def __init__(self, groups=frozenset()): Line 79: self._stopped = False should be renamed to 'stopped' as it is used (read) publically Line 80: if groups: Line 81: self._groups = groups Line 82: else: Line 83: self._groups = _GROUPS.keys() http://gerrit.ovirt.org/#/c/36197/27/tests/netlinkTests.py File tests/netlinkTests.py: Line 15: class NetlinkEventMonitorTests(TestCaseBase): Line 16: Line 17: @ValidateRunningAsRoot Line 18: def test_iterate_after_events(self): Line 19: with _timed_monitor(1) as mon: just a matter of style, but passing this as a named argumet might be more readable. Line 20: dummy_name = dummy.create() Line 21: dummy.remove(dummy_name) Line 22: for event in mon: Line 23: if event.get('name') == dummy_name: Line 18: def test_iterate_after_events(self): Line 19: with _timed_monitor(1) as mon: Line 20: dummy_name = dummy.create() Line 21: dummy.remove(dummy_name) Line 22: for event in mon: Just so I understand: when no events are collected by the monitor, does the test fails with MonitorError? If so, Does this flow (of not catching any events) worth yet another unit test? if not, will you consider testing a 0 events scenario? Line 23: if event.get('name') == dummy_name: Line 24: break Line 25: Line 26: @ValidateRunningAsRoot Line 29: iteration we start _add_device, which waits for iteration_ready event. Line 30: """ Line 31: dummy_name = dummy.create() Line 32: Line 33: def _add_device(): may rename this function to _add_and_remove? why dummy.create is outside of it's scope? Line 34: time.sleep(.2) Line 35: dummy.setLinkUp(dummy_name) Line 36: dummy.remove(dummy_name) Line 37: add_device_thread = threading.Thread(target=_add_device) Line 72: "to link or route." % event['event']) Line 73: Line 74: @ValidateRunningAsRoot Line 75: def test_iteration(self): Line 76: with _timed_monitor(1) as mon: * is this timeout enough even for slow Jenkins vms? * I think you should filter for dummy related events, so you won't get a StopIteration on lines 84 or 81 Line 77: iterator = iter(mon) Line 78: Line 79: # Generate events to avoid blocking Line 80: dummy_name = dummy.create() Line 123: % (1 + len(expected_events))) Line 124: Line 125: Line 126: @contextmanager Line 127: def _timed_monitor(time, groups=frozenset(), raise_exception=True): why renaming this to 'time'? Maybe 'interval' (the original Timer arg name) or 'timeout' is clearer? Line 128: mon = monitor.Monitor(groups=groups) Line 129: mon.start() Line 130: try: Line 131: timer = Timer(time, mon.stop) -- To view, visit http://gerrit.ovirt.org/36197 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0f4fcfde87ad51eb832f54862371b4da1281826e Gerrit-PatchSet: 27 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: [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
