[libvirt] [PATCH python] Improve quality of sanitytest check

[Doug Goldstein]

> 
> On Nov 27, 2013, at 6:19 AM, "Daniel P. Berrange" <berrange at redhat.com> wrote:
> 
>> On Tue, Nov 26, 2013 at 01:16:24PM -0600, Doug Goldstein wrote:
>> On Tue, Nov 26, 2013 at 12:32 PM, Daniel P. Berrange
>> <berrange at redhat.com> wrote:
>>> From: "Daniel P. Berrange" <berrange at redhat.com>
>>> 
>>> Validate that every public API method is mapped into the python
>>> and that every python method has a sane C API.
>> 
>> Looks like we had the same idea and even a similar approach as well.
>> 
>>> 
>>> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
>>> ---
>>> sanitytest.py | 309 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>>> setup.py      |  35 +++----
>>> 2 files changed, 294 insertions(+), 50 deletions(-)
>>> mode change 100755 => 100644 sanitytest.py
>>> 
>>> diff --git a/sanitytest.py b/sanitytest.py
>>> old mode 100755
>>> new mode 100644
>>> index 517054b..9e4c261
>>> --- a/sanitytest.py
>>> +++ b/sanitytest.py
>>> @@ -1,40 +1,283 @@
>>> #!/usr/bin/python
>>> 
>>> import sys
>>> +import lxml
>>> +import lxml.etree

Can we drop the explicit lxml import since we are only using etree? Then we can try lxml.etree and xml.etree as fallbacks. Do we need lxml added to the spec file for building as well?

>>> +import string
>>> 
>>> +# Munge import path to insert build location for libvirt mod
>>> sys.path.insert(0, sys.argv[1])
>>> -
>>> import libvirt
>>> +import libvirtmod
>> 
>> I wouldn't directly import libvirtmod due to Cygwin. I'd just use
>> libvirt.libvirtmod which is what its available as.
> 
> Ah, good point.
> 
>>> +# Phase 1: Identify all functions and enums in public API
>>> +set = tree.xpath('/api/files/file/exports[@type="function"]/@symbol')
>>> +for n in set:
>>> +    wantfunctions.append(n)
>>> +
>>> +set = tree.xpath('/api/files/file/exports[@type="enum"]/@symbol')
>>> +for n in set:
>>> +    wantenums.append(n)
>>> +
>> 
>> Maybe its a bit ugly but I actually grabbed the typedef's as well to
>> check the various namespaces (e.g. virConnect, virDomain) but not sure
>> if we want that.
> 
> I used the method names themselves to detect this. Could perhaps do
> both to have double the sanity test, but this can wait for now.
> 

Works for me. This is a giant leap forward for the tests.

>> 
>>> +
>>> +# Phase 2: Identify all classes and methods in the 'libvirt' python module
>>> +gotenums = []
>>> +gottypes = []
>>> +gotfunctions = { "libvirt": [] }
>>> +
>>> +for name in dir(libvirt):
>>> +    if name[0] == '_':
>>> +        continue
>>> +    thing = getattr(libvirt, name)
>>> +    if type(thing) == int:
>>> +        gotenums.append(name)
>>> +    elif type(thing) == type:
>>> +        gottypes.append(name)
>>> +        gotfunctions[name] = []
>>> +    elif callable(thing):
>>> +        gotfunctions["libvirt"].append(name)
>>> +    else:
>>> +       pass
>> 
>> Could the body of this be made into a function reused below?
> 
> Well the two loops are not really the same, so don't think
> it would save much code.
> 
>> 
>>> +
>>> +for klassname in gottypes:
>>> +    klassobj = getattr(libvirt, klassname)
>>> +    for name in dir(klassobj):
>>> +        if name[0] == '_':
>>> +            continue
>>> +        thing = getattr(klassobj, name)
>>> +        if callable(thing):
>>> +            gotfunctions[klassname].append(name)
>>> +        else:
>>> +            pass
> 
>> Just some visual comments until I get a chance to really play with
>> this. I stopped at the fixup area, which in my code is equally painful
>> as well. You're obviously a bit more knowledgable about Python than I
>> am because your fixups are a bit cleaner.
> 
> With all the fixes I've sent so far, I'm finally able to run this sanity
> test against builds of the python done against historical versions, which
> means we're getting alot better at compat.
> 

Overall ACK with the fix for Cygwin. It's a great improvement. The lxml import might be nice too. Sorry I don't have a specific example as I'm tapping this out on a phone in the car. 

--
Doug