Change in vdsm[master]: tests: Test storagetestlib FakeEnv functions

alitke at redhat.com alitke at redhat.com
Mon Apr 11 14:47:42 UTC 2016


Adam Litke has posted comments on this change.

Change subject: tests: Test storagetestlib FakeEnv functions
......................................................................


Patch Set 1:

(9 comments)

https://gerrit.ovirt.org/#/c/55889/1/tests/storagetestlibTests.py
File tests/storagetestlibTests.py:

Line 65:         with fake_file_env() as env:
Line 66:             set_domain_metaparams(env.sd_manifest,
Line 67:                                   {sd.DMDK_DESCRIPTION: 'foo'})
Line 68:             self.assertEqual('foo',
Line 69:                              env.sd_manifest.getMetaParam(sd.DMDK_DESCRIPTION))
> I'm not sure what do you test here - is this related to the fake env?
The idea is to test that setting the metadata in this instance of the domain manifest (basically the in-memory component of the persistent dict) worked.  Next, we'll use a different object to ensure that this metadata change will be "connected" to the new domain manifest instance we are creating.
Line 70: 
Line 71:             # Test that metadata is persisted to our temporary storage area
Line 72:             domain_dir = env.sd_manifest.domaindir
Line 73:             manifest = fileSD.FileStorageDomainManifest(domain_dir)


Line 70: 
Line 71:             # Test that metadata is persisted to our temporary storage area
Line 72:             domain_dir = env.sd_manifest.domaindir
Line 73:             manifest = fileSD.FileStorageDomainManifest(domain_dir)
Line 74:             self.assertEqual('foo', manifest.getMetaParam(sd.DMDK_DESCRIPTION))
> Do you want to test that our fake domain looks like a real domain?
Is this related to your question above or something different?
Line 75: 
Line 76:     def test_volume_metadata_io(self):
Line 77:         with fake_file_env() as env:
Line 78:             size = 1 * MB


Line 78:             size = 1 * MB
Line 79:             img_id, vol_id = make_file_volume(env.sd_manifest.domaindir, size)
Line 80:             vol = env.sd_manifest.produceVolume(img_id, vol_id)
Line 81:             vol.setDescription('foo')
Line 82:             self.assertEqual('foo', vol.getDescription())
> Why do we need this assert? we are not testing the storage domain manifest 
It's the same reason as for the domain test.  I am verifying that the property change first propagated to the object, then I will recreate the volume and ensure the change has persisted.  I can remove this check, but I think it helps to illustrate what is being tested (and will help to figure out where the error is should this test fail).
Line 83: 
Line 84:             # Test that metadata is persisted to our temporary storage area
Line 85:             vol = env.sd_manifest.produceVolume(img_id, vol_id)
Line 86:             self.assertEqual('foo', vol.getDescription())


Line 82:             self.assertEqual('foo', vol.getDescription())
Line 83: 
Line 84:             # Test that metadata is persisted to our temporary storage area
Line 85:             vol = env.sd_manifest.produceVolume(img_id, vol_id)
Line 86:             self.assertEqual('foo', vol.getDescription())
> I would be nice to use a constant for 'foo', same for the previous test.
Done
Line 87: 
Line 88: 
Line 89: class FakeBlockEnvTests(VdsmTestCase):
Line 90: 


Line 98:             md_path = env.lvm.lvPath(vg_name, sd.METADATA)
Line 99:             self.assertTrue(os.path.exists(md_path))
Line 100: 
Line 101:             # Check that the domain's special LVs can be found via LVM
Line 102:             special_lvs = sd.SPECIAL_VOLUME_SIZES_MIB.keys() + [sd.METADATA]
> Unneeded
Done
Line 103:             for lv in special_lvs:
Line 104:                 self.assertEqual(lv, env.lvm.getLV(vg_name, lv).name)
Line 105: 
Line 106:             images_dir = os.path.join(env.sd_manifest.domaindir, vg_name,


Line 99:             self.assertTrue(os.path.exists(md_path))
Line 100: 
Line 101:             # Check that the domain's special LVs can be found via LVM
Line 102:             special_lvs = sd.SPECIAL_VOLUME_SIZES_MIB.keys() + [sd.METADATA]
Line 103:             for lv in special_lvs:
> for lv in blockSD.SPECIAL_LVS:
Done
Line 104:                 self.assertEqual(lv, env.lvm.getLV(vg_name, lv).name)
Line 105: 
Line 106:             images_dir = os.path.join(env.sd_manifest.domaindir, vg_name,
Line 107:                                       sd.DOMAIN_IMAGES)


Line 118:             self.assertEqual(vol_id, env.lvm.getLV(sd_id, vol_id).name)
Line 119:             vol = env.sd_manifest.produceVolume(img_id, vol_id)
Line 120:             self.assertEqual(size / volume.BLOCK_SIZE, vol.getSize())
Line 121:             vol.setDescription('foo')
Line 122:             self.assertEqual('foo', vol.getDescription())
> This test the volume class, we don't need to test it here.
Done
Line 123: 
Line 124:             # Test that metadata is persisted to our temporary storage area
Line 125:             vol = env.sd_manifest.produceVolume(img_id, vol_id)
Line 126:             self.assertEqual('foo', vol.getDescription())


Line 122:             self.assertEqual('foo', vol.getDescription())
Line 123: 
Line 124:             # Test that metadata is persisted to our temporary storage area
Line 125:             vol = env.sd_manifest.produceVolume(img_id, vol_id)
Line 126:             self.assertEqual('foo', vol.getDescription())
> Use constant for 'foo'
Done
Line 127: 
Line 128:     def test_domain_metadata_io(self):
Line 129:         with fake_block_env() as env:
Line 130:             set_domain_metaparams(env.sd_manifest,


Line 129:         with fake_block_env() as env:
Line 130:             set_domain_metaparams(env.sd_manifest,
Line 131:                                   {sd.DMDK_DESCRIPTION: 'foo'})
Line 132:             self.assertEqual('foo',
Line 133:                              env.sd_manifest.getMetaParam(sd.DMDK_DESCRIPTION))
> I think we can remote this
Done
Line 134: 
Line 135:             # Test that metadata is persisted to our temporary storage area
Line 136:             sd_id = env.sd_manifest.sdUUID
Line 137:             manifest = blockSD.BlockStorageDomainManifest(sd_id)


-- 
To view, visit https://gerrit.ovirt.org/55889
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I65ab8c6419dfa1181efff2a533c00589ba5b8263
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list