[Libvirt-cim] [PATCH] [TEST] Fixing 41_cs_to_settingdefinestate.py CS tc

[Kaitlin Rupert]

> 4) Algined the tc to 80 columns.

Can you fix the comment block at the top of the test case?  These lines 
aren't formatted to 80 characters.

Can you also fix the indention for the Associators() call in 
get_associators_info().  The indention looks like the following:

         assoc_info = Associators(server,
                                      an,
                                      cn,
                     InstanceID = instid)

It should look like:

         assoc_info = Associators(server,
                                  an,
                                  cn,
                                  InstanceID = instid)


> +
> +
> +def get_SDS_verify_RASD_build_vssdc_input(server, virt, an, cs_cn, 
> +                                         rasd_values, sd_assoc_info):

This function is pretty big - can you break this up into smaller functions?

> 
> -        # Expect the SystemDevice to return 4 logical device records.
> -        # one each for memory, network, disk and processor and hence 4.
> -        # and hence expect the in_setting_define_state to contain just 4 entries.
> -        an  = get_typed_class(virt, "SystemDevice")
> +        # Expect the SystemDevice records == len(rasd_values) entries.
>          qcn = "Logical Devices"
> -        exp_len = 4
> +        exp_len = len(rasd_values) 
>          if check_len(an, in_setting_define_state, qcn, exp_len) != PASS:
>              return FAIL, in_setting_define_state

I know you need to check the len is several places, but the check_len() 
function is just a few lines of code.  I think having a function for 
this abstracts too much of the function into a separate function.

I would remove the check_len() and print_err() functions.  They're not 
really large enough to need their own functions.
> 
> -    status, cs_dom  = poll_for_state_change(server, virt, test_dom, 2, 
> -                                            timeout=10)
> -    if status != PASS and cs_dom.RequestedState != 0:
> -        vsxml.destroy(server)
> -        return FAIL
> +        an        = get_typed_class(virt, 'SystemDevice')
> +        qcn       = 'Logical Devices'

This spacing here is strange.  It should be:

an = get_typed_class(virt, 'SystemDevice')

One whitespace before and after the '='

> +        rasd_val, status = init_rasd_list(virt, server)
> +        status, in_vssdc_list = get_SDS_verify_RASD_build_vssdc_input(server,
> +                                                                      virt, an,
> +                                                                      cs_class,
> +                                                                      rasd_val,
> +                                                                      sd_assoc)
> +        exp_len = len(rasd_val) 
> +        if status !=PASS or check_len(an, in_vssdc_list, qcn, exp_len) != PASS:

Space between != and PASS.

> 
> -    # verify the results of SettingsDefineState with the cs_values list that was 
> -    # built using the output of the enumeration on ComputerSystem.
> -    status = verify_CS_values(cs_assoc_info, cs_values, an, qcn)
> +        # verify the results of SettingsDefineState with the cs_values list that was 

This line is longer than 80 characters.

I think main() is still a little difficult to read.  I would try to 
break it up into functions.  You could have something like the following:

1) system_device_assoc()  - do your verify in this function
2) sds_assoc()
3) vssdc_assoc()

You don't have to do it this way, but it's just a suggestion on how to 
break the test case up some.


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