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

[Mark Michelson]

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


In general, this looks like a good thing. The individual findings I have are quite few. There are some "un-pythonic" things going on in the code, the most common one being the way if statements are constructed. For instance, it's most common to have if statements that are "if something" or "if not something" when possible. So for instance, you could rewrite

if len(list):
    do something

to

if list:
    do something


and

if string != "":
    do something

to

if string:
    do something


and

if True == boolean:
    do something

to

if boolean:
    do something


Some other pythonistas may wish to chime in if there are other changes that may be warranted here.


/asterisk/team/sgriepentrog/testsuite-valgrind/lib/python/asterisk/asterisk.py
<https://reviewboard.asterisk.org/r/3027/#comment19660>

    I recommend outputting what the wrong output was.



/asterisk/team/sgriepentrog/testsuite-valgrind/runtests.py
<https://reviewboard.asterisk.org/r/3027/#comment19658>

    I think you left a word out in this log message.


- Mark Michelson


On Nov. 25, 2013, 11:10 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3027/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2013, 11:10 p.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/team/sgriepentrog/testsuite-valgrind/runtests.py 4350 
>   /asterisk/team/sgriepentrog/testsuite-valgrind/lib/python/asterisk/asterisk.py 4350 
>   /asterisk/team/sgriepentrog/testsuite-valgrind/lib/python/asterisk/TestRunner.py 4350 
>   /asterisk/team/sgriepentrog/testsuite-valgrind/lib/python/asterisk/TestConfig.py 4350 
>   /asterisk/team/sgriepentrog/testsuite-valgrind/lib/python/asterisk/TestCase.py 4350 
>   /asterisk/team/sgriepentrog/testsuite-valgrind/configs/valgrind.supp PRE-CREATION 
> 
> 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/20131126/246e9db8/attachment-0001.html>