Nir Soffer has posted comments on this change. Change subject: storage: add a context manager for the domainLock ......................................................................
Patch Set 1: Code-Review-1 (3 comments) Nice, the tests can be simplified. https://gerrit.ovirt.org/#/c/50272/1/tests/manifest_tests.py File tests/manifest_tests.py: Line 170: acquired = manifest._lvTagMetaSlotLock.acquire(False) Line 171: self.assertFalse(acquired) Line 172: Line 173: Line 174: class FakeStorageDomainManifest(sd.StorageDomainManifest): This is not really a fake object, but a subclass that good only for testing certain methods. I would call it TestingStorageDomainManifest Line 175: def __init__(self): Line 176: pass Line 177: Line 178: @recorded Line 192: with manifest.domain_lock(1): Line 193: expected_calls.append(("acquireDomainLock", (1,), {})) Line 194: self.assertEqual(manifest.__calls__, expected_calls) Line 195: expected_calls.append(("releaseDomainLock", (), {})) Line 196: self.assertEqual(manifest.__calls__, expected_calls) I would simplify like this - first, record another method: class FakeStorageDomainManifest(sd.StorageDomainManifest): ... @recorded def dummy(self): pass Now, lets call it with the context manager: with manifest.domain_lock(1): manifes.dummy() And verify that we got: [ ("acquireDomainLock", (1,), {}), ("dummy", (), {}), ("releaseDomainLock", (), {}), ] Line 197: Line 198: def test_domainlock_contextmanager_exception(self): Line 199: class InjectedFailure(Exception): Line 200: pass Line 207: self.assertEqual(manifest.__calls__, expected_calls) Line 208: raise InjectedFailure() Line 209: except InjectedFailure: Line 210: expected_calls.append(("releaseDomainLock", (), {})) Line 211: self.assertEqual(manifest.__calls__, expected_calls) I would test it like this: class FakeStorageDomainManifest(sd.StorageDomainManifest): ... def fail(self): raise ... Now, lets call it with the context manager: with manifest.domain_lock(1): manifes.fail() And verify that the exception was raised, and we recorded: [ ("acquireDomainLock", (1,), {}), ("releaseDomainLock", (), {}), ] -- To view, visit https://gerrit.ovirt.org/50272 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id7b831d4fe5a67f6998f31978f2399fdebdb3ceb Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Ala Hino <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
