Change in vdsm[master]: toolTests: change boolean logic.

nsoffer at redhat.com nsoffer at redhat.com
Sun Oct 12 22:06:02 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: toolTests: change boolean logic.
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.ovirt.org/#/c/34042/2/tests/toolTests.py
File tests/toolTests.py:

Line 59:     def __repr__(self):
Line 60:         return "name: %s, requires: %s" % (self._name, self._requires)
Line 61: 
Line 62:     def validate(self):
Line 63:         return self.should_succeed
This is the only improvement in this patch.
Line 64: 
Line 65:     def isconfigured(self):
Line 66:         if self.should_succeed:
Line 67:             return CONFIGURED


Line 64: 
Line 65:     def isconfigured(self):
Line 66:         if self.should_succeed:
Line 67:             return CONFIGURED
Line 68:         return NOT_CONFIGURED
This is the same as the previous code, or even little worse.

The pattern we like to see is:

    if error:
        fail
    continue the normal flow
Line 69: 
Line 70:     def configure(self):
Line 71:         if not self.should_succeed:
Line 72:             raise InvalidRun('mock for invalid configure')


Line 67:             return CONFIGURED
Line 68:         return NOT_CONFIGURED
Line 69: 
Line 70:     def configure(self):
Line 71:         if not self.should_succeed:
This previous code was more clear.
Line 72:             raise InvalidRun('mock for invalid configure')
Line 73: 
Line 74:     def removeConf(self):
Line 75:         if not self.should_succeed:


Line 71:         if not self.should_succeed:
Line 72:             raise InvalidRun('mock for invalid configure')
Line 73: 
Line 74:     def removeConf(self):
Line 75:         if not self.should_succeed:
This previous code was more clear.
Line 76:             raise Exception('mock invalid remove conf')
Line 77: 
Line 78: 
Line 79: class ConfiguratorTests(VdsmTestCase):


Line 187:         configurator,
Line 188:         '_CONFIGURATORS',
Line 189:         {
Line 190:             'a': MockModuleConfigurator('a', should_succeed=False),
Line 191:             'b': MockModuleConfigurator('b', should_succeed=False),
This is not better then the old code.
Line 192:         }
Line 193:     )
Line 194:     def testFailuresOnExposedFuncs(self):
Line 195:         self.assertRaises(


-- 
To view, visit http://gerrit.ovirt.org/34042
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6362c94ceee397908b03b5267363091ce892d024
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list