[Libvirt-cim] [PATCH] [TEST] Updating 01_forward.py of ECTP

[Kaitlin Rupert]

> +   
> +def  init_vs_pool_values(server, virt):

I don't think think this function is needed.  You don't need to confirm 
that the attribute values of the instances are valid - this should be 
covered by the class specific tests.  You just need to confirm that the 
association is returning the instances you expect - verify the values of 
the keys are as expected.

The code you have here is useful - however, I think it belongs in a 
class provider specific test, not an association provider test.

> +
> +                       
> +    return verify_ectp_list
> +
> +def verify_fields(assoc_val, pllst_index, vs_pool_values):

> +def verify_cs_hs_mig_fields(assoc_info, vs_pool_values):

Same goes for these 2 functions.  All of this can be condensed into a 
function that just checks key values.

> +def get_proflist(server, reg_classname, virt):
> +    proflist = []

I'd name this variable profiles_instid_list.

> +    status = PASS
> +    try: 
> +        key_list = ["InstanceID"]
> +        proflist = enumerate(server, reg_classname, key_list, virt) 
> +        if len(proflist) < 7:
> +            logger.error("'%s' returned '%d' '%s' objects, expected atleast 7", 
> +                         reg_classname, len(proflist), 'Profile')
> +            status = FAIL

> -    return PASS 
> +    if status != PASS:
> +        return status, proflist

And here, return profiles_instid_list instead of proflist.

> +
> +    profiles_instid_list = [ profile.InstanceID for profile in proflist ] 
> +
> +    return status, profiles_instid_list 
> +
> +

> 
>  @do_main(sup_types)
>  def main():
>      options = main.options
> -
> +    server  = options.ip
> +    virt    = options.virt
> +  
>      status = PASS
>      destroy_and_undefine_all(options.ip, options.virt)
> 
>      virt_xml = vxml.get_class(options.virt)
>      cxml = virt_xml(test_dom)
> -    ret = cxml.define(options.ip)
> +    ret = cxml.cim_define(server)

You'll want to check the return value of cim_define() to make sure the 
call doesn't fail.

> +    ret = cxml.start(server)
>      if not ret:
> -        logger.error('Unable to define domain %s' % test_dom)
> +        logger.error('Unable to start domain %s' % test_dom)
>          return FAIL
> +
> 
>      prev_namespace = Globals.CIM_NS
>      Globals.CIM_NS = 'root/interop'
> -    host = live.hostname(options.ip)
> 
> -    inst_lst = {
> -              "InstID1"  : "CIM:DSP1042-SystemVirtualization-1.0.0" ,
> -              "InstID2"  : "CIM:DSP1057-VirtualSystem-1.0.0a"
> -             }
> -    hs = get_typed_class(options.virt, "HostSystem")
> -    cs = get_typed_class(options.virt, "ComputerSystem")
> +    status = verify_ectp_assoc(server, virt)

I'm not much in favor of having the bulk of test be in a function. 
Functions are really helpful at breakup up long bits of code, but in 
this case, main setups up the environment, calls a function, and then 
tears down the environment.

In order to understand what the test does, you are forced to look at the 
verify_ectp_assoc().  It seems like a lot of this function could be 
included in the main portion.  You don't have to change this, but I 
think it's something to try to avoid in the future.

-- 
Kaitlin Rupert
IBM Linux Technology Center
kaitlin at linux.vnet.ibm.com