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

[Kaitlin Rupert]

>>
>> 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.

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.
-- 
Kaitlin Rupert
IBM Linux Technology Center
kaitlin at linux.vnet.ibm.com