9 Dec
2008
9 Dec
'08
1:53 a.m.
Kaitlin Rupert wrote:
>> + status = FAIL
I am changing the status = PASS here
>>
>> +
>> + # This check is required for libivirt-cim providers which do not have
>> + # CRS changes in it and the CRS provider is available with revision
>> >= 688.
>> + curr_cim_rev, changeset = get_provider_version(options.virt,
>> options.ip)
>> + if (curr_cim_rev < libvirtcim_hr_crs_changes):
>> + logger.info("ConsoleRedirectionService provider not supported, "
>> + "hence skipping the test ....")
>> + return SKIP
>> +
>> + name = "%s:%s" % ("1", "1")
>> + status = PASS
>
> Remove this line - status is set to FAIL above. It's better to set
> status to FAIL because we should only set it to PASS in cases when
> we're sure a PASS condition has been met.
>
Now, I have removed the line status = PASS .
>> +
>> + # Looping by passing invalid key values + for field, test_val in
>> tc_scen.items():
>> + newkey_vals = key_vals.copy()
>> + newkey_vals[test_val] = field
>> + ret_value = try_getinstance(conn, classname, newkey_vals,
>> + field_name=test_val, + expr_values = expr_values[field], + bug_no =
>> "")
>> + if ret_value != PASS:
>> + logger.error(" -------------- FAILED %s ----------- : " % field)
>> + break
>
> You capture the return of try_getinstance() as ret_value, but at the
> end of the test, you are returning status. If ret_value is FAIL, but
> somewhere earlier in the test, status is set to PASS, then you could
> potientially return a false positive.
>
> I would change ret_value to status here.
>
Thanks for finding this, now I have changed the status = ret_value .
>> +
>> + cxml.destroy(options.ip)
>
> This should be cim_destroy().
>
Also changed this to cim_destroy().
>
> Thanks for making all these changes! I think the flow of the test is
> much easier to read this time around.
Earlier I had based my testcase on an existing testcase in the cimtest.
Hence quite a few changes required I will submit the testcase freshly.
Thanks Kaitlin for the review . Please accept it.
Thanks
Veerendra C