[libvirt] [test-API PATCH 5/6] Cleanup and fix of domain/define test

[Martin Kletzander]

On 03/27/2012 12:35 PM, Guannan Ren wrote:
> On 03/27/2012 05:42 PM, Martin Kletzander wrote:
>> On 03/27/2012 10:59 AM, Guannan Ren wrote:
>>> On 03/27/2012 03:57 PM, Martin Kletzander wrote:
>>>>>            It's better not to use libvirt API to check the result
>>>>> of one
>>>>> another API.
>>>>>            We should use native approach to do the checking. So I
>>>>> insist
>>>>> on the original checking.
>>>>>
>>>> There was no lookupByName function, but I agree it's better to use the
>>>> same approach, so I'll add this method to the connection API.
>>>> One more question, whilst on this subject, though. I still didn't get
>>>> why we encapsulate libvirt API into one more class where we don't make
>>>> use of any persistence, inheritance nor any other OOP approach. It
>>>> would
>>>> help me to understand so I don't make future mistakes.
>>>          I think you don't need to add  lookupByName() in connectAPI.py
>>>      The APIs is domain related, so we suggest to make use of it in
>>> domainAPI.py
>>>      only. The ideas is that all of hypervisor related APIs go to
>>> connectAPI.py.
>>>          The relationship between classes domain, network, storage,
>>> nodedev is
>>>      loose. If we had a virtnetwork subclass, It would be better to make
>>> virtnetwork inherite
>>>      network for better OOP.
>>>          The encapsulated API files in lib directory is easy to manage
>>> and use. For example
>>>      If we want to write a domain testcases, we probably don't need to
>>> import network
>>>      and storage module in lib.
>>>
>> We both probably talk about something else, let me clarify.
>>
>> My apologies first, because I misunderstood that you wanted to use the
>> native approach. I thought you meant implementing the function in
>> connectAPI whether you meant checking on the machine without using
>> libvirt. That said, there is no point in talking about lookupByName
>> implementation for now.
>>
>> About the actual checking if the domain was created. Leaving it as it is
>> now, any misconfiguration will make this check fail (meaning the
>> configuration of sshd, libvirt, etc.). It will work for now as we are
>> the only ones using that right now (I guess), I'm just trying to think
>> ahead and I don't see that big problem with checking the domain being
>> created using libvirt again, but that's just my opinion.
>>
>> About the re-implementation of the API, I was just looking into the
>> connectAPI class for now, but what I see there is only constructor that
>> saves the connection from libvirt and then for each method of libvirts
>> connection, there is connectAPI method that does the same, just using
>> different method names (and raising different exception). Unfortunately
>> these aren't even consistent. For example:
>>
>> libvirts openAuth is encapsulated as openAuth
>> libvirts isEncrypted is encapsulated as isEncrypted
>>
>> but
>>
>> libvirts getCapabilities is encapsulated as get_caps
>> libvirts getHostname is encapsulated as get_host_name
>>
>> and so on.
>>
>> Don't get me wrong, I'm not trying to complain or anything, I'm just
>> trying to understand because for me it doesn't make any sense to use
>> this class.
> 
>       Ok, let me keep it temporarily, because it doesn't affect testing.
>     After some practice,  it proves to be useless,  we can remove them
> anytime.
>     Is that okay?
> 

No problem, we can keep it for now, or not even temporarily, I just
wanted to know if I missed something or misunderstood.

I'll be sending v2 for some of the patches anyway, so we'll keep this as
is and we can discuss it with some other major changes (if there will be
any).

Have a nice day
Martin