[Libvirt-cim] [PATCH] [TEST] Fixing the 01_verify_rasd_fields.py tc of RASD

[Deepti B Kalakeri]

Kaitlin Rupert wrote:
> > import sys
>> -from CimTest import Globals
>> from XenKvmLib.const import do_main
>> -from XenKvmLib.test_doms import destroy_and_undefine_all
>> from XenKvmLib import assoc
>> from XenKvmLib import vxml 
>
> Change this to XenKvmLib.vxml import get_class
>
>
>> + for rasd in assoc_info:
>> + guest, dev, status = parse_instance_id(rasd['InstanceID'])
>> + if status != PASS:
>> + logger.error("Unable to parse InstanceID: %s", rasd['InstanceID'])
>> + return status
>> +
>> + if guest != test_dom:
>> + continue
>
> Instead of a continue here, this should throw an error.
>
> VirtualSystemSettingDataComponent should only return RASDs that 
> correspond to a give VSSD. And a VSSD instance can only represent one 
> guest.
>
> This is a mistake I made in SettingsDefine/02_reverse.py, which is the 
> test I told you to reference. Sorry for the error here! I'll send a 
> patch to fix this bug.
>
> Really, we don't need to do this check at all. When I was re-writing 
> SettingsDefine/02_reverse.py, I was thinking that it is a good idea to 
> verify that the RASDs match the guest we're testing.
>
> But really, if VirtualSystemSettingDataComponent returns the wrong 
> instances, that's something the VirtualSystemSettingDataComponent 
> related tests should find. This test should be concerned with 
> verifying the RASD properties, it shouldn't be concerned with also 
> verifying the output of VirtualSystemSettingDataComponent.
>
> But it's still a valid check to have. So you can leave it in or remove 
> it. Either way is fine by me. =)
Yesthis is true. For now I have left the above check as it is, since the 
VSSDC test cases does not verify this as of now.
>
>> +
>> + logger.info("Verifying: %s", rasd.classname)
>> + exp_rasd = rasds[rasd.classname]
>> + for item in rasd.items():
>> + key = item[0]
>> + exp_val = eval('exp_rasd.' + key)
>> + ret_rasd_val = rasd[key]
>> + if ret_rasd_val == exp_val:
>> + status = PASS
>
> Will compare_all_prop() work for this? When we spoke, I said that 
> compare_all_prop() should be phased out once the association related 
> calls return the same object type as the enumeration related calls.
>
> However, since the the return types of the enumeration calls and the 
> association calls don't match yet, I'd rather compare_all_prop() be 
> used instead of duplicating the code here. When it comes time to 
> remove compare_all_prop(), it's easier to replace the function name 
> than it is to replace an entire code block.
>
> Sorry for the confusion there.
I thought since we plan to remove the compare_all_prop() sometime and 
since it is used in very few place, I wrote this above piece of code in 
the tc itself.
I have changed the above to use compare_all_prop().
>
>> else:
>> - status = 1
>> - if status != 0 or proc_status != 0 or net_status != 0 or \
>> - disk_status != 0 or mem_status != 0 :
>> - logger.error("Mistmatching association values" )
>> - status = 1 - except Exception, detail :
>> - logger.error("Exception in assoc_values function: %s" % detail)
>> - status = 1
>> - - return status
>> - + logger.info("Got %s instead of %s", ret_rasd_val, exp_val)
>> + status = FAIL
>> + break
>> +
>> + if status != PASS:
>> + logger.error("RASD with id %s not returned", exp_rasd.InstanceID)
>
> This message is misleading.. because you verify all the properties. So 
> even if the InstanceID matches, one of the other properties could be 
> different. I would use a more generic message to indicate that a 
> matching RASD wasn't found.
>
> The providers shouldn't return more than one RASD with the same 
> InstanceID. But it's possible it could happen in some kind of odd 
> error condition.
>
>>
>> virt_xml = vxml.get_class(virt)
>> if virt == 'LXC':
>> cxml = virt_xml(test_dom)
>> else:
>> cxml = virt_xml(test_dom, mem=test_mem, vcpus = test_vcpus,
>> - mac = test_mac, disk = test_disk)
>> - ret = cxml.create(options.ip)
>> + mac = test_mac)
>> + + ret = cxml.create(server)
>
> Can you change this to define() and cim_start().
>
Should have noticed this before itself.
Changed the tc to use define and start now.

Thanks and Regards,
Deepti.