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

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