[asterisk-dev] [Code Review] 3027: Valgrind support in TestSuite
Matt Jordan
reviewboard at asterisk.org
Wed Nov 27 11:37:26 CST 2013
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3027/#review10291
-----------------------------------------------------------
/asterisk/team/sgriepentrog/testsuite-valgrind/configs/valgrind.supp
<https://reviewboard.asterisk.org/r/3027/#comment19667>
Asterisk also comes with a valgrind.supp file in the contrib folder. You may want to compare this to that and see if there's anything else that should be included.
/asterisk/team/sgriepentrog/testsuite-valgrind/runtests.py
<https://reviewboard.asterisk.org/r/3027/#comment19661>
We already have a routine, __archive_ast_logs, which will walk the ast%d directories and, if they haven't been archived yet, will archive them to the test failure location. I would combine the __archive_ast_valgrind_logs with that - if the valgrind output exists, copy it over, otherwise, move on.
If you want to keep the counting of the Asterisk directories that's fine, but I don't think we need two separate loops that do the same thing.
/asterisk/team/sgriepentrog/testsuite-valgrind/runtests.py
<https://reviewboard.asterisk.org/r/3027/#comment19663>
It's very strange for us to call __parse_run_output twice here. We should only need to call it once - ideally, we would have the valgrind output that we want to display on stdout already ready for it.
/asterisk/team/sgriepentrog/testsuite-valgrind/runtests.py
<https://reviewboard.asterisk.org/r/3027/#comment19662>
I'd move all of the valgrind log file manipulation into its own class. It really is its own thing, and shouldn't be part of the TestRun class.
/asterisk/team/sgriepentrog/testsuite-valgrind/runtests.py
<https://reviewboard.asterisk.org/r/3027/#comment19664>
As commented previously, the act of walking the directories should only occur one time. There should be a single routine that archives all of the output.
/asterisk/team/sgriepentrog/testsuite-valgrind/runtests.py
<https://reviewboard.asterisk.org/r/3027/#comment19665>
Don't direct the output of errors to both stdout and print them directly. In general, we shouldn't call print - that overrides the user's choices in terms of logging and what is displayed.
/asterisk/team/sgriepentrog/testsuite-valgrind/runtests.py
<https://reviewboard.asterisk.org/r/3027/#comment19666>
As Mark suggested, this can be reduced by restructuring:
while os.path.isdir("%s/ast%d" % (ast_directories, i)):
...
if not os.path.exists(xml_file):
continue
etc.
- Matt Jordan
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/20131127/f0b36a3b/attachment-0001.html>
More information about the asterisk-dev
mailing list