[Libvirt-cim] [PATCH] [TEST]Fixing 06_paused_active_suspend.py tc failure

[Kaitlin Rupert]

> +    try:
> +
> +        for i in range(1, (timeout + 1)):
> +            sleep(1)
> +            cs = computersystem.get_cs_class(virt)(server, test_dom)
> +            if cs.Name == test_dom:
> +                to_RequestedState = cs.RequestedState
> +                enabledState =  cs.EnabledState
> +            else:
> +                logger.error("VS %s not found" % test_dom)
> +                return status 

You'll want to break here, otherwise the guest doesn't get destroyed 
properly.

> +            if enabledState == expstate:
> +                status = PASS
> +                break

Instead of breaking here, you can just return since we've found the 
value we're looking for.

> - 
> +
> +    status, to_RequestedState, enabledState = poll_for_status(options.ip, options.virt, cxml, 2)
> +    if status != PASS or enabledState != 2: 

No need to verify enabledState == 2, the poll_for_status function do 
this already.

> +        return status
> +     
>  #Suspend the VS
> -    rc = call_request_state_change(test_dom, options.ip, REQUESTED_STATE,
> -                                   TIME, options.virt)
> +    rc = call_request_state_change(test_dom, options.ip, REQUESTED_STATE, TIME, options.virt)

This change causes the test to span multiple lines.  I'll work on 
sending out some suggested style guidelines and post them to the list to 
see if people agree.


> -    except Exception, detail:
> -        logger.error("Exception variable: %s" % detail)
> -        return status
> -
> +    status, to_RequestedState, enabledState = poll_for_status(options.ip, options.virt, cxml, 
> +                                                                                 FINAL_STATE)

Unusual formatting here.  Usually, the second line of arguments for a 
function will line up with the first argument.  Something like:

status, to_RequestedState, enabledState = poll_for_status(options.ip, 
options.virt, cxml, 


                                                           FINAL_STATE)

Hopefully the formatting of the email doesn't kill the illustration.

>      if enabledState != FINAL_STATE:
>          logger.error("EnabledState has %i instead of %i", enabledState, FINAL_STATE)
>          logger.error("Try to increase the timeout and run the test again")
> 
>      if status != PASS:
> -        ret = cxml.destroy(options.ip)
> -        cxml.undefine(options.ip)

Why remove the destroy and undefine here?  The guest won't get cleaned 
up properly.

>          return status
> 
>  # Success: 
> @@ -134,9 +144,8 @@ def main():
>  # To state == 2
>  # Enabled_state == RequestedState
> 
> -    if from_State == START_STATE and \
> -        to_RequestedState == FINAL_STATE and \
> -        enabledState == to_RequestedState:
> +    if from_State == START_STATE and to_RequestedState == FINAL_STATE and \
> +                                        enabledState == to_RequestedState:

This is unusual indention style.  Usually, the new line starts just 
below the if or the first part of the statement.  So you'd have:

if from_State == START_STATE and to_RequestedState == FINAL_STATE and \
enabledState == to_RequestedState:

or

if from_State == START_STATE and to_RequestedState == FINAL_STATE and \
    enabledState == to_RequestedState:

My preference is the latter.  You can hold off on changing this patch 
until we agree on a coding style.

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