[asterisk-dev] [Code Review] 3027: Valgrind support in TestSuite

Matt Jordan reviewboard at asterisk.org
Thu Dec 12 16:14:59 CST 2013 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3027/#review10406
-----------------------------------------------------------



/asterisk/trunk/lib/python/asterisk/TestCase.py
<https://reviewboard.asterisk.org/r/3027/#comment19822>

    First, spaces should be used between parameters.
    
    Second, this code change has nothing to do with adding valgrind support. This is attempting to fix a failure in a test that is not part of the review. While trying to provide a fail safe in case someone has an exception in a deferred error is laudable, it also effectively masks a critical error in someone's test code.
    
    The TestCase class now pipes twisted logging to the Python logger. That has the effect of showing to the user in the log files when twisted hits an exception in a deferred error callback or other place. I'd prefer to have people fix the actual problems in the test, rather than masking it.
    
    



/asterisk/trunk/lib/python/asterisk/TestConfig.py
<https://reviewboard.asterisk.org/r/3027/#comment19825>

    You shouldn't need to do this.
    
    YAML should allow you to specify boolean values for properties. That means that you don't need to treat that as string literals, case sensitivty stuff, etc.
    
    Instead, you should be able to do:
    
    self.valgrind = properties.get("valgrind") or False
    
    Which is much more Pythonic, handles "valgrind" not being specified in properties, etc.
    
    In general, I think the TestConfig class makes a mistake as well in trying to mask configuration errors. If someone specifies an invalid value for valgrind, the Test Suite should throw an exception. You gave it something bad. Right now, if I specified:
    
    valgrind=foobar
    
    The test would still run, although without valgrind (maybe I thought foobar would work?) - which is probably not what I intended.



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19821>

    There's a lot of PEP 8 compliance issues here that should be addressed.
    
    http://www.python.org/dev/peps/pep-0008/
    
    While a fair chunk of the older portions of the Asterisk Test Suite are non-PEP 8 compliant, for some time now we've been working to adhere better to the guidelines. New code should attempt to adhere to it wherever possible.
    
    A few obvious ones in this file:
    
    * Modules should have short, all-lowercase names (valgrind.py)
    * Function names should be lowercase, with words separated by underscores as necessary to improve readability.
    * Global variables should be all uppercase
    * Classes should inherit from object



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19823>

    In general, code should be documented using Python docstrings.
    
    http://www.python.org/dev/peps/pep-0257/
    
    I'd take a look at the conventions used in the rest of the Test Suite for how classes, methods, and other code is documented. I can't say we're the best at consistency here, but there are a few different acceptable styles (we should probably standardize this at some point):
    
    """Summary
    
    Keyword Arguments:
    arg1 Foos the widget
    
    Returns:
    The widget
    """
    
    """Summary
    
    :param arg1: Foos the widget
    :returns: The widget
    """
    
    



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19829>

    From a class design perspective, class attributes that are public are non-prefixed with '_', private are prefixed with '_', and - to avoid name collisions with subclasses, can also be prefixed with '__'.
    
    See section 9.6: http://docs.python.org/2/tutorial/classes.html
    
    In this particular case, if there are properties you don't want to be changed by something that uses this class, they should be made private.



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19830>

    I would handle the indexing of Asterisk directories a bit differently. More on that later.
    
    But index should probably be removed. Requiring its manipulation outside of this class before calling InsertValgrindCmd feels non-intuitive.



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19826>

    I'm not sure I understand this comment. Why is TestRun referenced here?
    
    In what situation would test_config be None?



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19824>

    This is a bit strange to me.
    
    The constructor for TestConfig



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19828>

    You can simplify this slightly:
    
    self.leak = test_config.valgrind_leak or os.getenv("VALGRINDLEAK")
    
    This can be applied to all of your parameter extractions.



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19827>

    I would remove this option.
    
    (1) It doesn't work with all tests, which means that it has very limited use. It also feels bad to have a function that is called by command line options that potentially won't work - we've handed someone a tool that may drive in a nail, or it may just hit them in the thumb repeatedly.
    
    (2) Since the Test Suite already generates backtraces from core files, a crash already results in a substantial amount of information. If inspection of the core file is desired, the code that deletes the core file can be commented out (which is a one liner). This is typically only ever done in extreme debugging scenarios, and doesn't have a lot of utility in general.
    



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19831>

    The object calling this method knows which Asterisk instance it wants. 



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19832>

    I'm not sure why index is being used here like this, or why it is called index.
    
    If non-zero and/or set (which is only done externally on line 197 of TestCase), then - if also are using GDB - we'll start GDB up on that instance of Asterisk in two seconds.
    
    The behavior here feels very odd however - I would expect that if GDB was supported, we would run all Asterisk instances under GDB - at which point, it's moot to pass index to this function.
    This means that:
    
    (1) Tests that have a single instance of Asterisk won't have GDB started on them
    (2) Tests that use more than one instance will have instances 1+ have GDB started (if GDB is enabled)
    
    If we still wanted to use GDB - which I don't think we should - then this should just be a parameter passed to this function. Index should not be a class attribute.



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19839>

    In general, this routine is rather hard to follow. That is typical when hand parsing an XML DOM in any programming language.
    
    Since the goal here is to convert an XML DOM into some human readable format, I have to wonder: why not XSLT? Python's etree library allows you to transform a DOM given an XSLT in just a few lines:
    
    dom = ET.parse(xml_filename)
    xslt = ET.parse(xsl_filename)
    transform = ET.XSLT(xslt)
    newdom = transform(dom)
    print(ET.tostring(newdom, pretty_print=True))
    
    As it is, I'm not sure we want to invest any more time in this, so I wouldn't say this has to be done. It should be considered however for any future manipulation of the XML output, or for any other XML reports we find ourselves facing. Anytime you find yourself walking a DOM to format it, you should ask yourself: is there a better tool for this?
    
    (Corollary: XML is like violence. If it isn't solving your problems, you just aren't using enough)



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19838>

    I'm curious why we need this try/except here.
    
    We've already done all we can to make sure that the file is a well formed DOM. At this point, if there's an exception, things should be pretty far off the rails.
    
    I'd prefer to have an exception in the XML parsing bubble up and get fixed, rather than handling it here.



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19836>

    Since this gets rather indented rather quickly, I would structure this to try and reduce the indentation:
    
    xmldoc = None
    excp = None
    
    while (...):
    
    if not os.path.exists(xml_file):
        continue
    
    try:
        xmldoc = xml.dom.minidom.parse(xml_file)
    except Exception as e:
        excp = e
        with open (xml_file, "a") as xmlfile:
            xmlfile.write("</valgrindoutput>")
    
    if not xmldoc:
        try:
            xmldoc = xml.dom.minidom.parse(xml_file)
        except Exception:
            pass
    
    if not xmldoc:
        msg = "Exception while parsing %s: %s" % (xml_file, excp)
        ...
        continue
    
    self.parse_xml_doc(xmldoc, i)
    
    It requires a bit more care, but it also keeps exception handlers from nesting inside more exception handlers, which always feels like things have gotten a bit crazy.



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19835>

    Spaces should always go between operators and their operands.
    
    This applies throughout the file, as there's a few locations where this occurs.



/asterisk/trunk/lib/python/asterisk/Valgrind.py
<https://reviewboard.asterisk.org/r/3027/#comment19837>

    Pretty sure I had this finding previously, but in general, classes should not be printing. Classes should be sending their output to the Python logger, which has the ability to redirect error messages to stdout.
    
    The only exception are classes that are created prior to test execution (such as the TestConfig class), as the Python logger is not always initialized by that point.



/asterisk/trunk/lib/python/asterisk/asterisk.py
<https://reviewboard.asterisk.org/r/3027/#comment19840>

    You don't need max_wait_time as a variable here.
    
    I would only bump this to 20 seconds if valgrind is enabled. Otherwise, this will wait a lot longer than necessary before stopping the test.



/asterisk/trunk/lib/python/asterisk/asterisk.py
<https://reviewboard.asterisk.org/r/3027/#comment19841>

    Why is this being set? There's no real context shown for why we'd set the environment variable at this point.
    
    If this is needed by valgrind, I'd assume it would only be done if valgrind support was enabled.


- Matt Jordan


On Dec. 10, 2013, 11:51 a.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3027/
> -----------------------------------------------------------
> 
> (Updated Dec. 10, 2013, 11:51 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> This patch adds support for running Asterisk under Valgrind (say: Val-Grinned) to check for all sorts of nasty runtime bugs.  This started off with a post to asterisk-dev list by nitesh.bansal at gmail.com, and he wrote and contributed the initial version.  So if you find this useful, be sure to thank him.  Then I made extensive changes and additions, so if the code stinks, blame me.
> 
> The following has been done:
> 
> - Check runtests.py arguments for --valgrind and --valgrind-gensupp flags, pass via environ to TestCase.py
> - Note previously existing instances of ast# logs to insure we only process new log/xml files
> - Increase reactor timeout by x5 when valgrind enabled
> - Patch to reactor_stop() to insure it does even when exceptions occur in deferred stack
> - Add valgrind with correct arguments into executable path
> - After run, check valgrind.xml for errors, parse and condense them into something more managable
> - If -gensup mode enabled, write suppressions to logs/(test)/ast#/valgrind.supp that can be added (manually)
> - A default valgrind.supp "suppressions" file is in configs/ to prevent complaints about known unfixables
> 
> Notes:
> 
> - valgrind can be triggered by argument to runtests.py, export VALGRIND=true, or in test-config.yaml
> - configs/valgrind.supp will be used if found, but tests/(test)/configs/valgrind.supp will take precedence
> - valgrind-gensupp mode will create example suppressions file, during which no suppressions occur
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/runtests.py 4350 
>   /asterisk/trunk/lib/python/asterisk/asterisk.py 4350 
>   /asterisk/trunk/lib/python/asterisk/Valgrind.py PRE-CREATION 
>   /asterisk/trunk/lib/python/asterisk/TestConfig.py 4350 
>   /asterisk/trunk/lib/python/asterisk/TestCase.py 4350 
>   /asterisk/trunk/configs/valgrind.supp PRE-CREATION 
>   /asterisk/trunk/README.txt 4350 
> 
> Diff: https://reviewboard.asterisk.org/r/3027/diff/
> 
> 
> Testing
> -------
> 
> Tested on 64bit and 32bit CentOS, including low cpu power conditions.  Some sporadic timing issues affecting AMI/twisted operation have been seen and will be corrected.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131212/b92394c6/attachment-0001.html>

More information about the asterisk-dev mailing list