Francesco Romani has posted comments on this change. Change subject: utils: add weakmethod helper ......................................................................
Patch Set 4: (10 comments) https://gerrit.ovirt.org/#/c/51865/4/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 905: func = meth.__func__ Line 906: ref = weakref.ref(meth.__self__) Line 907: Line 908: def wrapper(*args, **kwargs): Line 909: # TODO: if ref() is None? > +1 on the InvalidatedWeakref Will use InvalidatedWeakRef, which is nice indeed. Line 910: return func(ref(), *args, **kwargs) Line 911: https://gerrit.ovirt.org/#/c/51865/4/tests/utilsTests.py File tests/utilsTests.py: Line 998: Line 999: def public(self, *args, **kw): Line 1000: return 'public', args, kw Line 1001: Line 1002: def _private(self, *args, **kw): > We don't need this for the test, we are testing only wrapped method. Right, will remove. Line 1003: return '_private', args, kw Line 1004: Line 1005: def __del__(self): Line 1006: print('__del__', self.__class__.__name__) Line 1009: @contextlib.contextmanager Line 1010: def _debug_gc(): Line 1011: flags = gc.get_debug() Line 1012: try: Line 1013: gc.set_debug(gc.DEBUG_LEAK) > gc.DEBUG_SAVEALL is enough for the test, we don't want to write to stderr, will switch to DEBUG_SAVEALL Line 1014: yield Line 1015: finally: Line 1016: gc.set_debug(flags) Line 1017: Line 1015: finally: Line 1016: gc.set_debug(flags) Line 1017: Line 1018: Line 1019: def _broken_wrapper(meth): > We can define this in the single test that needs it. will move as local function in the test. Line 1020: Line 1021: def wrapper(*args, **kwargs): Line 1022: return meth(*args, **kwargs) Line 1023: Line 1023: Line 1024: return wrapper Line 1025: Line 1026: Line 1027: def _wrap(obj, method_wrapper): > We don't need to duplicate the logic in libvirtconnection.wrapMethod, it wi ok, will do locally. Good idea to test wrapMethod, but I'll save for a later patch. Line 1028: for name in dir(obj): Line 1029: if name.startswith('_'): Line 1030: continue Line 1031: attr = getattr(obj, name) Line 1030: continue Line 1031: attr = getattr(obj, name) Line 1032: if not callable(attr): Line 1033: continue Line 1034: setattr(obj, name, method_wrapper(attr)) > Lets move this into the test. ok (see above) Line 1035: del name, attr Line 1036: return obj Line 1037: Line 1038: Line 1041: def test_with_reference_cycle(self): Line 1042: Line 1043: # test environment is too noisy Line 1044: gc.collect() Line 1045: before = set(gc.garbage) > Do we have garbage in the tests without setting gc.DEBUG_SAVEALL? I think so, but not sure. Will check. Line 1046: Line 1047: with _debug_gc(): Line 1048: obj = _wrap(FakeVirDomain(), _broken_wrapper) Line 1049: Line 1049: Line 1050: self.assertEquals(obj.public(), ("public", (), {})) Line 1051: self.assertEquals(obj._private(), ("_private", (), {})) Line 1052: Line 1053: del obj > We can simplify this: Neat, will use this. Line 1054: Line 1055: gc.collect() Line 1056: after = set(gc.garbage) Line 1057: Line 1057: Line 1058: self.assertIn(FakeVirDomain, Line 1059: (inst.__class__ for inst in (after - before))) Line 1060: Line 1061: def test_without_reference_cycle(self): > Or use the permutations decorator? will try to simplify and reduce duplication Line 1062: Line 1063: # test environment is too noisy Line 1064: gc.collect() Line 1065: before = set(gc.garbage) Line 1069: Line 1070: self.assertEquals(obj.public(), ("public", (), {})) Line 1071: self.assertEquals(obj._private(), ("_private", (), {})) Line 1072: Line 1073: del obj > Can be simplified like the previous test: Agreed Line 1074: Line 1075: gc.collect() Line 1076: after = set(gc.garbage) Line 1077: -- To view, visit https://gerrit.ovirt.org/51865 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f26aa314e26142122e9f594275406cf7fbade98 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches