[Libvirt-cim] [PATCH] [TEST] Fixing 04_hs_to_EAPF.py tc

[Deepti B Kalakeri]

Kaitlin Rupert wrote:
>>>
>>> I know this was already existing code, but if pool_assoc doesn't 
>>> contain the instances you're expecting, then in_pllist won't contain 
>>> the proper values to query against ElementAllocatedFromPool.
>> I agree that the loop is little hard to read, but considering the 
>> fact that this is a Cross class test case where we would like to be 
>> able to as input for the next step as much as possible, do you think 
>> that manually generating a list of our own will serve the purpose ?
>> If yes, then the Crosss Class tc will no longer require query from 
>> the *HostedResourcePool, *correct??
>
> This is a really good point. I won't considering this a cross class 
> test when I first read the patch.
>
>>
>> def pool_init_list(virt, pool_assoc, net_name, dp_InstID):
>> """
>> Creating the lists that will be used for comparisons.
>> """
>> in_pllist = {}
>> dp_CName = get_typed_class(virt, 'DiskPool')
>> pp_CName = get_typed_class(virt, 'ProcessorPool')
>> mp_CName = get_typed_class(virt, 'MemoryPool')
>> np_CName = get_typed_class(virt, 'NetworkPool')
>> np_InstID = '%s/%s' % ('NetworkPool', net_name)
>>
>> for p_inst in pool_assoc:
>> CName = p_inst.classname
>> InstID = p_inst['InstanceID']
>> if CName == np_CName and InstID == np_InstID:
>> in_pllist[CName] = InstID
>> elif CName == dp_CName and InstID == dp_InstID:
>> in_pllist[CName] = InstID
>> elif CName == pp_CName or CName == mp_CName:
>> in_pllist[CName] = InstID
>> return in_pllist
>>
>
> What confuses me here are the comparisons: The outcome of each if 
> statement is the same. You're essentially building a list that is:
>
> in_pllist = { Cname : InstID,
> ...
> }
>
> Why only verify the network pool and disk pool InstanceIDs but not the 
> memory pool or processor pool InstanceIDs? If you want to verify 
> in_pllist only has InstanceIDs you're expecting, you should check for 
> proc and mem as well.
>
Since the processorpool and memorypool InstanceID do not change I did 
not consider checking them as well.
> Why not do something like:
>
> exp_pllist = { dp_CName : dp_InstID,
> ...
> }
>
> for p_inst in pool_assoc:
> CName = p_inst.classname
> InstID = p_inst['InstanceID']
> if exp_pllist[Cname] == InstID:
> in_pllist[Cname] = InstID
>
> This way, you can build a list of expected values. in_pllist should 
> match exp_pllist. You could then call check_len() at this point, or 
> you could call it after pool_init_list() returns.
>
> Although, this seems silly. You could do something like:
>
> for p_inst in pool_assoc:
> CName = p_inst.classname
> InstID = p_inst['InstanceID']
> if exp_pllist[Cname] != InstID:
> return FAIL, None
>
> return PASS, exp_pllist
>
> I agree that you want to use the output from the previous provider. 
> However, if you've already confirmed that A == B, then returning A is 
> no different than returning B.
>
> But either way is fine in this case. I just think the multiple if 
> statements in the loop were confusing - it took me awhile to 
> understand what that loop was trying to accomplish.
>
We should be returning the new list B that will be created since the 
HostedResourcePool might not contain all the Pool details that exp_list 
is expecting to have.
These are good approach :) thanks. I sent a new patch with the changes.

Thanks and Regards,
Deepti.