-----------------------------------------------------------
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.ban...@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
>
>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
