Nir Soffer has posted comments on this change. Change subject: StorageDomainManifest: introduce class heirarchy ......................................................................
Patch Set 8: (10 comments) https://gerrit.ovirt.org/#/c/41993/8/tests/manifestTests.py File tests/manifestTests.py: Line 18: # Line 19: Line 20: import os Line 21: import shutil Line 22: import tempfile shutil and tempfile not needed if we use testlib infrastructure. Line 23: import uuid Line 24: Line 25: from testlib import VdsmTestCase as TestCaseBase Line 26: from monkeypatch import MonkeyPatchScope Line 21: import shutil Line 22: import tempfile Line 23: import uuid Line 24: Line 25: from testlib import VdsmTestCase as TestCaseBase No need to rename VdsmTestCase Line 26: from monkeypatch import MonkeyPatchScope Line 27: Line 28: from storage import blockSD, fileSD, lvm Line 29: from storage.persistentDict import PersistentDict, DictValidator Line 27: Line 28: from storage import blockSD, fileSD, lvm Line 29: from storage.persistentDict import PersistentDict, DictValidator Line 30: Line 31: SDBLKSZ = 512 Don't we have this in the actual code somewhere? Line 32: Line 33: # /tmp may use tempfs filesystem, not suitable for some of the test assuming a Line 34: # filesystem with direct io support. Line 35: TEMPDIR = '/var/tmp' Line 31: SDBLKSZ = 512 Line 32: Line 33: # /tmp may use tempfs filesystem, not suitable for some of the test assuming a Line 34: # filesystem with direct io support. Line 35: TEMPDIR = '/var/tmp' use testlib.TEMPDIR, or better testlib.namedTemporaryDir() Line 36: Line 37: Line 38: class FakeBlockSDMetadata(object): Line 39: def __init__(self): Line 45: def writelines(self, metadata): Line 46: self._md = {} Line 47: for line in metadata: Line 48: k, v = line.split('=') Line 49: self._md[k] = v If all this does is writing and reading lines, maybe we can use a list: def readlines(self): return self.metadata def writelines(self, metadata): self.metadata = tuple(metadata) Line 50: Line 51: BlockSDMetadata = lambda: DictValidator( Line 52: PersistentDict(FakeBlockSDMetadata()), Line 53: blockSD.BLOCK_SD_MD_FIELDS) Line 56: class FileManifestTests(TestCaseBase): Line 57: VOLSIZE = 1024 Line 58: Line 59: def setUp(self): Line 60: self.mountpoint = tempfile.mkdtemp(dir=TEMPDIR) This creates temporary directory. If some code bellow raises, nobody is going to clean up the temporary directory - this is a well known design issue with setUp/tearDown in all python testing frameworks. Line 61: sdUUID = str(uuid.uuid4()) Line 62: self.domainPath = os.path.join(self.mountpoint, sdUUID) Line 63: self.manifest = fileSD.FileStorageDomainManifest(self.domainPath) Line 64: Line 71: def tearDown(self): Line 72: shutil.rmtree(self.mountpoint) Line 73: Line 74: def testGetReadDelay(self): Line 75: self.assertIsInstance(self.manifest.getReadDelay(), float) Lets add this helper that can be useful for other tests: @contextmanager def metadata_file(...): with testlib.namedTempraryDir() as tmpdir: create metadata file... yield path class tests... def test_foo(self): with metadata_file(...) as path: create manifest... test it... Line 76: Line 77: Line 78: class BlockManifestTests(TestCaseBase): Line 79: VOLSIZE = 1024 Line 84: self.manifest = blockSD.BlockStorageDomainManifest(sdUUID) Line 85: self.manifest.activate(BlockSDMetadata()) Line 86: Line 87: def tearDown(self): Line 88: shutil.rmtree(self.mountpoint) Same issue as above, use namedTemporaryDir() Line 89: Line 90: def testGetReadDelay(self): Line 91: dummyFile = os.path.join(self.mountpoint, 'dummy') Line 92: open(dummyFile, "w").truncate(self.VOLSIZE * SDBLKSZ) Line 92: open(dummyFile, "w").truncate(self.VOLSIZE * SDBLKSZ) Line 93: Line 94: # We patch lvm so lvPath returns our dummy file. Line 95: with MonkeyPatchScope([(lvm, 'lvPath', lambda *unused: dummyFile)]): Line 96: self.assertIsInstance(self.manifest.getReadDelay(), float) We probably need similar helper for the block tests. https://gerrit.ovirt.org/#/c/41993/8/tests/volumeTests.py File tests/volumeTests.py: Line 28 Line 29 Line 30 Line 31 Line 32 This seems unused now, delete? -- To view, visit https://gerrit.ovirt.org/41993 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia275424a8d423722efb229a4d8545f035fe5fa51 Gerrit-PatchSet: 8 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: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
