Change in vdsm[master]: properties: Add properties module

nsoffer at redhat.com nsoffer at redhat.com
Thu Dec 10 23:07:57 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: properties: Add properties module
......................................................................


Patch Set 6:

(8 comments)

https://gerrit.ovirt.org/#/c/40822/6/lib/vdsm/properties.py
File lib/vdsm/properties.py:

Line 119:         if not required and default not in values:
Line 120:             raise ValueError("Default value %s not in allowed values %s" %
Line 121:                              (default, values))
Line 122:         super(Enum, self).__init__(required=required, default=default, doc=doc)
Line 123:         self.values = values
Check that all values of of the same type.
Line 124: 
Line 125:     def validate(self, value):
Line 126:         if value not in self.values:
Line 127:             raise ValueError("Invalid value %r for property %s" %


Line 172: 
Line 173: class Float(_Number):
Line 174: 
Line 175:     def validate(self, value):
Line 176:         if not isinstance(value, float):
Should accept also integers:

    if not isinstance(value, (float, int)):
        ...
Line 177:             raise ValueError("Invalid value %r for float property %s" %
Line 178:                              (value, self.name))
Line 179:         return super(Float, self).validate(value)
Line 180: 


https://gerrit.ovirt.org/#/c/40822/6/tests/propertiesTests.py
File tests/propertiesTests.py:

Line 74:             self.value = value
Line 75: 
Line 76:     def test_required(self):
Line 77:         # Required property set to None is considered unset
Line 78:         self.assertRaises(ValueError, self.Cls)
> Should we add a test like so?
Probably yes.
Line 79: 
Line 80: 
Line 81: class PropertyDefaultTests(VdsmTestCase):
Line 82: 


Line 112:         obj = self.Cls()
Line 113:         self.assertEqual(obj.value, "2")
Line 114: 
Line 115:     def test_allowed(self):
Line 116:         for value in ("1", "2", "3"):
> Can an enum have values of mixed types (eg. [1, "2", u"3"]) ?  I know it's 
I guess we cannot and should not support this.

We need more tests.
Line 117:             obj = self.Cls()
Line 118:             obj.value = value
Line 119:             self.assertEqual(obj.value, value)
Line 120: 


Line 121:     def test_forbidden(self):
Line 122:         obj = self.Cls()
Line 123:         self.assertRaises(ValueError, setattr, obj, "value", "4")
Line 124: 
Line 125:     def test_none(self):
> So None is always allowed even if it was not specified in the values list? 
I think I changed it to make it easier to handle missing value in a dict as unset value:

    class Foo:

        value = properties.Integer()

        def __init__(self, d):
            self.value = d.get("foo")

You don't want to do:

    if "foo" in d:
        self.value = d["foo"]

Properties exists to prevent this boilerplate.
Line 126:         obj = self.Cls()
Line 127:         obj.value = None
Line 128:         self.assertEqual(None, obj.value)
Line 129: 


Line 270:         self.assertEqual(obj.value, 3.14)
Line 271: 
Line 272:     def test_invalid(self):
Line 273:         obj = self.Cls()
Line 274:         self.assertRaises(ValueError, setattr, obj, "value", "not a float")
> Are integers allowed?  ie. 3 ?  It would be nice to allow 3 without needing
No, I check for float. I'm not big fan of type automatic type conversion, but treating integers as floats should be safe and should be added.

Need to add a test.
Line 275: 
Line 276:     def test_none(self):
Line 277:         obj = self.Cls()
Line 278:         obj.value = None


Line 290:         self.assertEqual(1.0, obj.value)
Line 291: 
Line 292:     def test_too_small(self):
Line 293:         obj = self.Cls()
Line 294:         self.assertRaises(ValueError, setattr, obj, "value", -1.0)
> You might want to test with a value that is really, really close to the min
Aren't <  or >  good enough for comparing floats?

See the _Number class in properties.py
Line 295: 
Line 296: 
Line 297: class FloatMaxValueTests(VdsmTestCase):
Line 298: 


Line 350: 
Line 351:     def test_none(self):
Line 352:         obj = self.Cls()
Line 353:         obj.value = None
Line 354:         self.assertEqual(None, obj.value)
> Test with 0, 1, [], {}, and other costructs that are implicitly interpreted
Anything which is not True or False will raise ValueError, see test_invalid. But yes, we need more tests, maybe with permutations.
Line 355: 
Line 356: 
Line 357: class BooleanDefaultTests(VdsmTestCase):
Line 358: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I62175172dddce7f84319e3c8669747994e06a697
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Daniel Erez <derez at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list