On Mon, Aug 5, 2013 at 3:29 PM, Thomas Thrainer <[email protected]> wrote:
> > > > On Mon, Aug 5, 2013 at 2:45 PM, Michele Tartara <[email protected]>wrote: > >> On Mon, Aug 5, 2013 at 2:41 PM, Thomas Thrainer <[email protected]>wrote: >> >>> >>> >>> >>> On Mon, Aug 5, 2013 at 1:20 PM, Michele Tartara <[email protected]>wrote: >>> >>>> On Thu, Aug 1, 2013 at 11:18 AM, Thomas Thrainer >>>> <[email protected]>wrote: >>>> >>>>> Some configuration objects are accessed quite often, so introduce >>>>> shortcut properties for those. >>>>> >>>>> Signed-off-by: Thomas Thrainer <[email protected]> >>>>> --- >>>>> test/py/cmdlib/cluster_unittest.py | 46 >>>>> +++++++++++---------------- >>>>> test/py/cmdlib/test_unittest.py | 14 ++++---- >>>>> test/py/cmdlib/testsupport/cmdlib_testcase.py | 7 ++++ >>>>> 3 files changed, 33 insertions(+), 34 deletions(-) >>>>> >>>> >>>> >>>> Shouldn't these be introduced earlier given that they are part of the >>>> same patchset? >>>> >>> >>> Yeah, they could. But actually I wanted to save some work by not faking >>> history ;-). >>> I would have to distribute this change over a couple of other commits, >>> making sure in the progress that all tests continue to work after each >>> commit. Given the fact that I changed and refined the test framework >>> throughout the patch series again and again, I considered this change as a >>> refinement too, which does not need to be hidden away... >>> >> >> I'm saying that just because sending already clean patches is the default >> approach that has always been used in the past... >> > > Hmm, true. In this case, I probably should re-shape the whole patch series > again, in which the first commits add the test framework in its final > state, and only then actual tests are added. I'm not quite sure if this has > advantages, as I guess it's even harder to review a patch series where a > lot of support code or extractions are already made before the actual need > is visible. > In the past I remember some patch series where in early patches code > duplication or other "bad" style was introduced, only to have it corrected > in later patches of the series (especially from Helga, I guess this matches > her style of work as well). I don't know what the "official" opinion about > such patch series is. > > The ultimate goal, IMHO, should be to make the life of the developer and > the reviewer as easy as possible. I like during reviews to have some > context (which new requirements required a change, etc.), but that's just a > matter of taste. > > I see multiple options for this case: > - re-shape the whole patch series to be "framework-first" > - only redistribute this "shortcut properties" patch over the other ones > - leave it as is > > What do you think? Should we figure out some guidelines for patch series? > Or are there already guidelines I don't know of? > > As far as I know, this is the guideline that was used when I started submitting patches, I guess to keep the history as clean as possible. And that's the reason why I replied with this to your patch. Personally, I'd be in favor of changing that to allow things being implemented in one way at the beginning of a patch set and corrected before the end of it series, without the need for spreading the changes all over the original patches, given that it's quite time consuming. And probably, changing the code in later patches doesn't really do any harm, provided the code is as it should be by the end of the patch set. Let's see if Guido (or anybody else) has some suggestion on how we should deal with this. > >> >>> >>> >>>> >>>> >>>>> >>>>> diff --git a/test/py/cmdlib/cluster_unittest.py >>>>> b/test/py/cmdlib/cluster_unittest.py >>>>> index 95b4c9f..6061440 100644 >>>>> --- a/test/py/cmdlib/cluster_unittest.py >>>>> +++ b/test/py/cmdlib/cluster_unittest.py >>>>> @@ -273,21 +273,19 @@ class >>>>> TestLUClusterActivateMasterIp(CmdlibTestCase): >>>>> >>>>> self.rpc.call_node_activate_master_ip.return_value = \ >>>>> RpcResultsBuilder(cfg=self.cfg) \ >>>>> - .CreateSuccessfulNodeResult(self.cfg.GetMasterNode()) >>>>> + .CreateSuccessfulNodeResult(self.master) >>>>> >>>>> self.ExecOpCode(op) >>>>> >>>>> self.rpc.call_node_activate_master_ip.assert_called_once_with( >>>>> - self.cfg.GetMasterNode(), >>>>> - self.cfg.GetMasterNetworkParameters(), >>>>> - False) >>>>> + self.master_uuid, self.cfg.GetMasterNetworkParameters(), False) >>>>> >>>>> def testFailure(self): >>>>> op = opcodes.OpClusterActivateMasterIp() >>>>> >>>>> self.rpc.call_node_activate_master_ip.return_value = \ >>>>> RpcResultsBuilder(cfg=self.cfg) \ >>>>> - .CreateFailedNodeResult(self.cfg.GetMasterNode()) \ >>>>> + .CreateFailedNodeResult(self.master) \ >>>>> >>>>> self.ExecOpCodeExpectOpExecError(op) >>>>> >>>>> @@ -298,21 +296,19 @@ class >>>>> TestLUClusterDeactivateMasterIp(CmdlibTestCase): >>>>> >>>>> self.rpc.call_node_deactivate_master_ip.return_value = \ >>>>> RpcResultsBuilder(cfg=self.cfg) \ >>>>> - .CreateSuccessfulNodeResult(self.cfg.GetMasterNode()) >>>>> + .CreateSuccessfulNodeResult(self.master) >>>>> >>>>> self.ExecOpCode(op) >>>>> >>>>> self.rpc.call_node_deactivate_master_ip.assert_called_once_with( >>>>> - self.cfg.GetMasterNode(), >>>>> - self.cfg.GetMasterNetworkParameters(), >>>>> - False) >>>>> + self.master_uuid, self.cfg.GetMasterNetworkParameters(), False) >>>>> >>>>> def testFailure(self): >>>>> op = opcodes.OpClusterDeactivateMasterIp() >>>>> >>>>> self.rpc.call_node_deactivate_master_ip.return_value = \ >>>>> RpcResultsBuilder(cfg=self.cfg) \ >>>>> - .CreateFailedNodeResult(self.cfg.GetMasterNode()) \ >>>>> + .CreateFailedNodeResult(self.master) \ >>>>> >>>>> self.ExecOpCodeExpectOpExecError(op) >>>>> >>>>> @@ -328,7 +324,7 @@ class TestLUClusterConfigQuery(CmdlibTestCase): >>>>> >>>>> self.rpc.call_get_watcher_pause.return_value = \ >>>>> RpcResultsBuilder(self.cfg) \ >>>>> - .CreateSuccessfulNodeResult(self.cfg.GetMasterNode(), -1) >>>>> + .CreateSuccessfulNodeResult(self.master, -1) >>>>> >>>>> ret = self.ExecOpCode(op) >>>>> >>>>> @@ -365,7 +361,7 @@ class TestLUClusterDestroy(CmdlibTestCase): >>>>> >>>>> self.ExecOpCode(op) >>>>> >>>>> - self.assertSingleHooksCall([self.cfg.GetMasterNodeName()], >>>>> + self.assertSingleHooksCall([self.master.name], >>>>> "cluster-destroy", >>>>> constants.HOOKS_PHASE_POST) >>>>> >>>>> @@ -376,7 +372,7 @@ class TestLUClusterPostInit(CmdlibTestCase): >>>>> >>>>> self.ExecOpCode(op) >>>>> >>>>> - self.assertSingleHooksCall([self.cfg.GetMasterNodeName()], >>>>> + self.assertSingleHooksCall([self.master.name], >>>>> "cluster-init", >>>>> constants.HOOKS_PHASE_POST) >>>>> >>>>> @@ -390,7 +386,7 @@ class TestLUClusterQuery(CmdlibTestCase): >>>>> def testIPv6Cluster(self): >>>>> op = opcodes.OpClusterQuery() >>>>> >>>>> - self.cfg.GetClusterInfo().primary_ip_family = >>>>> netutils.IP6Address.family >>>>> + self.cluster.primary_ip_family = netutils.IP6Address.family >>>>> >>>>> self.ExecOpCode(op) >>>>> >>>>> @@ -430,18 +426,14 @@ class TestLUClusterRename(CmdlibTestCase): >>>>> >>>>> self.assertEqual(1, self.ssh_mod.WriteKnownHostsFile.call_count) >>>>> self.rpc.call_node_deactivate_master_ip.assert_called_once_with( >>>>> - self.cfg.GetMasterNode(), >>>>> - self.cfg.GetMasterNetworkParameters(), >>>>> - False) >>>>> + self.master_uuid, self.cfg.GetMasterNetworkParameters(), False) >>>>> self.rpc.call_node_activate_master_ip.assert_called_once_with( >>>>> - self.cfg.GetMasterNode(), >>>>> - self.cfg.GetMasterNetworkParameters(), >>>>> - False) >>>>> + self.master_uuid, self.cfg.GetMasterNetworkParameters(), False) >>>>> >>>>> def testRenameOfflineMaster(self): >>>>> op = opcodes.OpClusterRename(name=self.NEW_NAME) >>>>> >>>>> - self.cfg.GetMasterNodeInfo().offline = True >>>>> + self.master.offline = True >>>>> self.netutils_mod.GetHostname.return_value = \ >>>>> HostnameMock(self.NEW_NAME, self.NEW_IP) >>>>> >>>>> @@ -455,7 +447,7 @@ class TestLUClusterRepairDiskSizes(CmdlibTestCase): >>>>> self.ExecOpCode(op) >>>>> >>>>> def _SetUpInstanceSingleDisk(self, dev_type=constants.LD_LV): >>>>> - pnode = self.cfg.GetMasterNodeInfo() >>>>> + pnode = self.master >>>>> snode = self.cfg.AddNewNode() >>>>> >>>>> inst = self.cfg.AddNewInstance() >>>>> @@ -472,7 +464,7 @@ class TestLUClusterRepairDiskSizes(CmdlibTestCase): >>>>> >>>>> self.rpc.call_blockdev_getdimensions.return_value = \ >>>>> RpcResultsBuilder(cfg=self.cfg) \ >>>>> - .CreateFailedNodeResult(self.cfg.GetMasterNode()) >>>>> + .CreateFailedNodeResult(self.master) >>>>> >>>>> self.ExecOpCode(op) >>>>> >>>>> @@ -484,7 +476,7 @@ class TestLUClusterRepairDiskSizes(CmdlibTestCase): >>>>> >>>>> self.rpc.call_blockdev_getdimensions.return_value = \ >>>>> RpcResultsBuilder(cfg=self.cfg) \ >>>>> - .CreateSuccessfulNodeResult(self.cfg.GetMasterNode(), >>>>> node_data) >>>>> + .CreateSuccessfulNodeResult(self.master, node_data) >>>>> >>>>> return self.ExecOpCode(op) >>>>> >>>>> @@ -522,7 +514,7 @@ class TestLUClusterRepairDiskSizes(CmdlibTestCase): >>>>> >>>>> def testExclusiveStorageInvalidResultData(self): >>>>> self._SetUpInstanceSingleDisk() >>>>> - >>>>> self.cfg.GetMasterNodeInfo().ndparams[constants.ND_EXCLUSIVE_STORAGE] = >>>>> True >>>>> + self.master.ndparams[constants.ND_EXCLUSIVE_STORAGE] = True >>>>> self._ExecOpClusterRepairDiskSizes([(1024 * 1024 * 1024, None)]) >>>>> >>>>> self.mcpu.assertLogContainsRegex( >>>>> @@ -531,13 +523,13 @@ class >>>>> TestLUClusterRepairDiskSizes(CmdlibTestCase): >>>>> def testExclusiveStorageCorrectSpindles(self): >>>>> (_, disk) = self._SetUpInstanceSingleDisk() >>>>> disk.spindles = 1 >>>>> - >>>>> self.cfg.GetMasterNodeInfo().ndparams[constants.ND_EXCLUSIVE_STORAGE] = >>>>> True >>>>> + self.master.ndparams[constants.ND_EXCLUSIVE_STORAGE] = True >>>>> changed = self._ExecOpClusterRepairDiskSizes([(1024 * 1024 * >>>>> 1024, 1)]) >>>>> self.assertEqual(0, len(changed)) >>>>> >>>>> def testExclusiveStorageWrongSpindles(self): >>>>> self._SetUpInstanceSingleDisk() >>>>> - >>>>> self.cfg.GetMasterNodeInfo().ndparams[constants.ND_EXCLUSIVE_STORAGE] = >>>>> True >>>>> + self.master.ndparams[constants.ND_EXCLUSIVE_STORAGE] = True >>>>> changed = self._ExecOpClusterRepairDiskSizes([(1024 * 1024 * >>>>> 1024, 1)]) >>>>> self.assertEqual(1, len(changed)) >>>>> >>>>> diff --git a/test/py/cmdlib/test_unittest.py >>>>> b/test/py/cmdlib/test_unittest.py >>>>> index 9abc7bf..37e2b6b 100644 >>>>> --- a/test/py/cmdlib/test_unittest.py >>>>> +++ b/test/py/cmdlib/test_unittest.py >>>>> @@ -48,7 +48,7 @@ class TestLUTestDelay(CmdlibTestCase): >>>>> self.ExecOpCodeExpectOpExecError(op) >>>>> >>>>> def testOnNodeUuid(self): >>>>> - node_uuids = [self.cfg.GetMasterNode()] >>>>> + node_uuids = [self.master_uuid] >>>>> op = opcodes.OpTestDelay(duration=DELAY_DURATION, >>>>> on_node_uuids=node_uuids) >>>>> self.ExecOpCode(op) >>>>> @@ -57,19 +57,19 @@ class TestLUTestDelay(CmdlibTestCase): >>>>> >>>>> def testOnNodeName(self): >>>>> op = opcodes.OpTestDelay(duration=DELAY_DURATION, >>>>> - on_nodes=[self.cfg.GetMasterNodeName()]) >>>>> + on_nodes=[self.master.name]) >>>>> self.ExecOpCode(op) >>>>> >>>>> - >>>>> >>>>> self.rpc.call_test_delay.assert_called_once_with([self.cfg.GetMasterNode()], >>>>> + >>>>> self.rpc.call_test_delay.assert_called_once_with([self.master_uuid], >>>>> DELAY_DURATION) >>>>> >>>>> def testSuccessfulRpc(self): >>>>> op = opcodes.OpTestDelay(duration=DELAY_DURATION, >>>>> - on_nodes=[self.cfg.GetMasterNodeName()]) >>>>> + on_nodes=[self.master.name]) >>>>> >>>>> self.rpc.call_test_delay.return_value = \ >>>>> RpcResultsBuilder(cfg=self.cfg) \ >>>>> - .AddSuccessfulNode(self.cfg.GetMasterNode()) \ >>>>> + .AddSuccessfulNode(self.master) \ >>>>> .Build() >>>>> >>>>> self.ExecOpCode(op) >>>>> @@ -78,11 +78,11 @@ class TestLUTestDelay(CmdlibTestCase): >>>>> >>>>> def testFailingRpc(self): >>>>> op = opcodes.OpTestDelay(duration=DELAY_DURATION, >>>>> - on_nodes=[self.cfg.GetMasterNodeName()]) >>>>> + on_nodes=[self.master.name]) >>>>> >>>>> self.rpc.call_test_delay.return_value = \ >>>>> RpcResultsBuilder(cfg=self.cfg) \ >>>>> - .AddFailedNode(self.cfg.GetMasterNode()) \ >>>>> + .AddFailedNode(self.master) \ >>>>> .Build() >>>>> >>>>> self.ExecOpCodeExpectOpExecError(op) >>>>> diff --git a/test/py/cmdlib/testsupport/cmdlib_testcase.py >>>>> b/test/py/cmdlib/testsupport/cmdlib_testcase.py >>>>> index fb22a06..a52a958 100644 >>>>> --- a/test/py/cmdlib/testsupport/cmdlib_testcase.py >>>>> +++ b/test/py/cmdlib/testsupport/cmdlib_testcase.py >>>>> @@ -71,6 +71,13 @@ class CmdlibTestCase(testutils.GanetiTestCase): >>>>> REMOVE = object() >>>>> IGNORE = object() >>>>> >>>>> + cluster = property(fget=lambda self: self.cfg.GetClusterInfo(), >>>>> + doc="Cluster configuration object") >>>>> + master = property(fget=lambda self: self.cfg.GetMasterNodeInfo(), >>>>> + doc="Master node") >>>>> + master_uuid = property(fget=lambda self: self.cfg.GetMasterNode(), >>>>> + doc="Master node UUID") >>>>> + >>>>> def setUp(self): >>>>> super(CmdlibTestCase, self).setUp() >>>>> self._iallocator_patcher = None >>>>> -- >>>>> 1.8.3 >>>>> >>>>> >>>> >>>> >>>> Thanks, >>>> Michele >>>> >>>> -- >>>> Google Germany GmbH >>>> Dienerstr. 12 >>>> 80331 München >>>> >>>> Registergericht und -nummer: Hamburg, HRB 86891 >>>> Sitz der Gesellschaft: Hamburg >>>> Geschäftsführer: Graham Law, Christine Elizabeth Flores >>>> >>> >>> >>> >>> -- >>> Thomas Thrainer | Software Engineer | [email protected] | >>> >>> Google Germany GmbH >>> Dienerstr. 12 >>> 80331 München >>> >>> Registergericht und -nummer: Hamburg, HRB 86891 >>> Sitz der Gesellschaft: Hamburg >>> Geschäftsführer: Graham Law, Christine Elizabeth Flores >>> >> >> Michele >> >> -- >> Google Germany GmbH >> Dienerstr. 12 >> 80331 München >> >> Registergericht und -nummer: Hamburg, HRB 86891 >> Sitz der Gesellschaft: Hamburg >> Geschäftsführer: Graham Law, Christine Elizabeth Flores >> > > > > -- > Thomas Thrainer | Software Engineer | [email protected] | > > Google Germany GmbH > Dienerstr. 12 > 80331 München > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores > Michele -- Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
