Nir Soffer has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 2:
(4 comments)
https://gerrit.ovirt.org/#/c/47857/2/tests/storagefakelib.py File tests/storagefakelib.py:
Line 26: Line 27: Line 28: class FakeLVM(object): Line 29: _PV_SIZE = 10 << 30 # We pretend all PVs are 10G in size Line 30: _PV_PE_SIZE = 128 << 20 # Found via inspection of real environment
But is the PE size always the same as the PE metadata size? If you used a
I think that extent size is the smallest unit you can manage in lvm, and metadata area will use whole extent. I think it is safe for now to assume this is true. Line 31: _PV_MDA_COUNT = 2 # The number of PEs used for metadata areas Line 32: _PV_UNUSABLE = (_PV_PE_SIZE * # 2 PE for metadata + 1 PE to hold a header Line 33: (1 + _PV_MDA_COUNT)) Line 34:
Line 27: Line 28: class FakeLVM(object): Line 29: _PV_SIZE = 10 << 30 # We pretend all PVs are 10G in size Line 30: _PV_PE_SIZE = 128 << 20 # Found via inspection of real environment Line 31: _PV_MDA_COUNT = 2 # The number of PEs used for metadata areas
It's hard-coded in _createpv
ok :-) Line 32: _PV_UNUSABLE = (_PV_PE_SIZE * # 2 PE for metadata + 1 PE to hold a header Line 33: (1 + _PV_MDA_COUNT)) Line 34: Line 35: def __init__(self, root):
https://gerrit.ovirt.org/#/c/47857/2/tests/storagefakelibTests.py File tests/storagefakelibTests.py:
Line 66: lvm = FakeLVM(tmpdir) Line 67: lvm.createVG(self.VG_NAME, self.DEVICES, Line 68: blockSD.STORAGE_UNREADY_DOMAIN_TAG, Line 69: blockSD.VG_METADATASIZE) Line 70: yield lvm
I fail to see why moving this to a separate class is any neater. I like ha
Having setup code for each fixture makes sense, but combining it with code to create temporary directory and fake lvm and returning fake lvm from the context manager is less convincing. Line 71: Line 72: @permutations([ Line 73: ['name', VG_NAME], Line 74: ['size', '10334765056'],
Line 86: ]) Line 87: def test_vg_properties(self, var, val): Line 88: with self.base_config() as lvm: Line 89: vg = lvm.getVG(self.VG_NAME) Line 90: self.assertEqual(val, getattr(vg, var))
I wrote it this way because in the sdm indirection tests you suggested that
I think we don't need permutations when we have one fixture. The is little chance that initialization of the fake lv will fail, as there is very little logic there, and using one permutation for each tested value means we are creating 13 temporary directories for checking contents of a dict.
You can test all values in one test, or maybe separate the few values that has some logic, like you did with the attributes.
Lets try to make these tests simplest as possible. Line 91: Line 92: @permutations([ Line 93: ['permission', 'w'], Line 94: ['resizeable', 'z'],