Adam Litke has posted comments on this change. Change subject: storage: Introduce guarded utilities ......................................................................
Patch Set 11: (18 comments) https://gerrit.ovirt.org/#/c/61435/11/tests/Makefile.am File tests/Makefile.am: Line 222: stompAsyncClientTests.py \ Line 223: stompAsyncDispatcherTests.py \ Line 224: stompTests.py \ Line 225: storageMailboxTests.py \ Line 226: storage_guarded_test.py \ > The tests should work with python 3 if we remove the dependency on storage Actually we still depend on storagetestlib which depends on storagefakelib which depends on lvm. lvm is not Python 3 compatible so it will not work. I don't want to try and fix this now. Line 227: storage_hsm_test.py \ Line 228: storage_monitor_test.py \ Line 229: storageServerTests.py \ Line 230: storage_rwlock_test.py \ https://gerrit.ovirt.org/#/c/61435/11/tests/storage_guarded_test.py File tests/storage_guarded_test.py: Line 43: Line 44: class ContextTest(VdsmTestCase): Line 45: Line 46: def inject_failure(self, *args): Line 47: raise InjectedFailure() > Any reason to use an instance method instead of a function in the module? nope. Changing. Line 48: Line 49: def test_none(self): Line 50: with guarded.context(): Line 51: pass Line 46: def inject_failure(self, *args): Line 47: raise InjectedFailure() Line 48: Line 49: def test_none(self): Line 50: with guarded.context(): > Better force the caller to send empty lock list Done Line 51: pass Line 52: Line 53: def test_one_vol(self): Line 54: defs = [('dom', 'img', 'vol')] Line 79: ('release', '02_img', 'img2', 'mode'), Line 80: ('release', '02_img', 'img1', 'mode'), Line 81: ('release', '01_dom', 'dom2', 'mode'), Line 82: ('release', '01_dom', 'dom1', 'mode')] Line 83: with guarded.context(*locks): > If we simplify the context interface to receive list of locks, we don't nee Done Line 84: self.assertEqual(expected[:6], log) Line 85: self.assertEqual(expected, log) Line 86: Line 87: def test_two_vols_same_image(self): Line 103: def test_acquire_failure(self): Line 104: defs = [('dom1', 'img1', 'vol1')] Line 105: log = [] Line 106: locks = self.make_lock_lists(defs, log) Line 107: locks[0][2].acquire = self.inject_failure # Fail on vol lock > It is not clear what lock is locks[0][2]. Creating the locks here in a simp Great idea! Changing. Line 108: expected = [('acquire', '01_dom', 'dom1', 'mode'), Line 109: ('acquire', '02_img', 'img1', 'mode'), Line 110: ('release', '02_img', 'img1', 'mode'), Line 111: ('release', '01_dom', 'dom1', 'mode')] Line 114: pass Line 115: except InjectedFailure: Line 116: self.assertEqual(expected, log) Line 117: else: Line 118: self.fail("InjectedFailure not raised") > We can simplify these tests like this: Done Line 119: Line 120: def test_aquire_failure_then_release_failure(self): Line 121: defs = [('dom1', 'img1', 'vol1')] Line 122: log = [] Line 131: pass Line 132: except InjectedFailure: Line 133: self.assertEqual(expected, log) Line 134: else: Line 135: self.fail("InjectedFailure not raised") > Same, simplify with assertRaises Done Line 136: Line 137: def test_release_failure(self): Line 138: defs = [('dom1', 'img1', 'vol1')] Line 139: log = [] Line 163: ('release', '02_img', 'img1', 'mode'), Line 164: ('release', '01_dom', 'dom1', 'mode')] Line 165: try: Line 166: with guarded.context(*locks): Line 167: self.inject_failure() > We can just raise here, no need to call the helper. Done Line 168: except InjectedFailure: Line 169: self.assertEqual(expected, log) Line 170: else: Line 171: self.fail("InjectedFailure not raised") Line 167: self.inject_failure() Line 168: except InjectedFailure: Line 169: self.assertEqual(expected, log) Line 170: else: Line 171: self.fail("InjectedFailure not raised") > Same, simplify with assertRaises Done Line 172: Line 173: def test_fail_inside_context_with_release_failure(self): Line 174: def _error(): Line 175: raise RuntimeError() Line 188: _error() Line 189: except RuntimeError: Line 190: self.assertEqual(expected, log) Line 191: else: Line 192: self.fail("RuntimeError not raised") > Same, simplify using assertRaises Done Line 193: Line 194: def make_lock_lists(self, defs, log): Line 195: ret = [] Line 196: for sd, img, vol in defs: Line 193: Line 194: def make_lock_lists(self, defs, log): Line 195: ret = [] Line 196: for sd, img, vol in defs: Line 197: ret.append(self.make_locks(sd, img, vol, log)) > Just create one list with all locks, we don't really need lists of lock lis Removed this helper altogether. Line 198: return ret Line 199: Line 200: def make_locks(self, sd_id, img_id, vol_id, log): Line 201: return [ Line 196: for sd, img, vol in defs: Line 197: ret.append(self.make_locks(sd, img, vol, log)) Line 198: return ret Line 199: Line 200: def make_locks(self, sd_id, img_id, vol_id, log): > I don't think this helper is good idea - yes, the tests are shorter this wa Done Line 201: return [ Line 202: FakeGuardedLock('01_dom', sd_id, 'mode', log), Line 203: FakeGuardedLock('02_img', img_id, 'mode', log), Line 204: FakeGuardedLock('03_vol', vol_id, 'mode', log), Line 255: lock.release() Line 256: self.assertEqual(expected, log) Line 257: Line 258: Line 259: class ResourceManagerLockTest(VdsmTestCase): > Should be tested separately in the module implementing it (resourceManager. Done Line 260: Line 261: def test_acquire_release(self): Line 262: res_mgr = FakeResourceManager() Line 263: Line 283: pass Line 284: Line 285: Line 286: @expandPermutations Line 287: class VolumeLeaseTest(VdsmTestCase): > Should be tested separately in the module implementing it (sd.py?) Done Line 288: Line 289: def test_acquire_release(self): Line 290: sdcache = FakeStorageDomainCache() Line 291: manifest = FakeSDManifest() https://gerrit.ovirt.org/#/c/61435/11/tests/storagetestlib.py File tests/storagetestlib.py: Line 359: return vol_list Line 360: Line 361: Line 362: class FakeGuardedLock(guarded.AbstractLock): Line 363: def __init__(self, ns, name, mode, log=None): > Why not require log? Done Line 364: self._ns = ns Line 365: self._name = name Line 366: self._mode = mode Line 367: self._log = log Line 380: Line 381: def acquire(self): Line 382: if self._log is not None: Line 383: entry = ('acquire', self._ns, self._name, self._mode) Line 384: self._log.append(entry) > I would do: Done Line 385: Line 386: def release(self): Line 387: if self._log is not None: Line 388: entry = ('release', self._ns, self._name, self._mode) Line 382: if self._log is not None: Line 383: entry = ('acquire', self._ns, self._name, self._mode) Line 384: self._log.append(entry) Line 385: Line 386: def release(self): > Same for release Done Line 387: if self._log is not None: Line 388: entry = ('release', self._ns, self._name, self._mode) Line 385: Line 386: def release(self): Line 387: if self._log is not None: Line 388: entry = ('release', self._ns, self._name, self._mode) Line 389: self._log.append(entry) > It would be nice to test FakeGuardedLock, just couple of stupid tests. They were already in storage_guarded_test.py but moving to the storagetestlibTests -- To view, visit https://gerrit.ovirt.org/61435 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2b0a204818d44b6205515277f4c2834cb2b7a057 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@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/admin/lists/vdsm-patches@lists.fedorahosted.org
