<p>George Joseph <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6845">View Change</a></p><p>Patch set 3:</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I'm not a real fan of having to add a parameter to every test's<br>yaml.  How about making this a command line option that<br>automatically adds<br>'n' amount to very reactor timeout?<br>> --reactor-timeout-adjustment=5 to add 5 seconds, or if you have a<br>really fast machine,  --reactor-timeout-adjustment=-5 to reduce<br>the<br>timeout by 5 seconds.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I think you're misunderstanding the purpose of this patch.  Reactor<br>timeout is a side-effect, not the main point.  Reactor timeout is<br>only touched to deal with the case where a test needs >30 second<br>shutdown delay.  So if we have memcheck-delay-stop: 90 to deal with<br>a minse timer we increase the reactor-timeout to 93 seconds minimum<br>to avoid showing reactor timeout messages while the test is<br>stopping.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This patch is not intended to deal with any slowness caused by<br>valgrind or REF_DEBUG (code to do that globally is already in<br>place).  The point is to wait long enough for any relevant timers<br>to fire and cleanup resources.  SIP timer_b takes just as long to<br>fire with or without memory checking, the only difference is that<br>we care enough to wait for it when we are checking memory.</p><p style="white-space: pre-wrap; word-wrap: break-word;">In my own testing this memcheck-delay-stop will only be needed on<br>about half the PJSIP tests and is unneeded by nearly all non-PJSIP<br>tests.  Many of the PJSIP tests will need 7 seconds (assuming<br>timer_b is configured to minimum).  Some tests will need 60 seconds<br>(ast_sip_session_defer_termination), others might need 90 seconds<br>(minse configured to minimum).  I had considered setting the<br>default memcheck-delay-stop to 7 seconds but this would have<br>drawbacks.  A 7 second delay for every test would increase the<br>run-time of the full testsuite by nearly 2 hours (1000 tests). <br>Maybe 200 of the tests need any delay.  Most cases we're dealing<br>with timer_b which means that we need to reconfigured timer_t1 +<br>timer_b or use memcheck-delay-stop: 32, so for a 7 second delay to<br>ever be effective we'd need to touch the pjsip.conf files anyways.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Ok, can you update the commit message to explain all that.</p><ul style="list-style: none; padding-left: 20px;"></ul><p>To view, visit <a href="https://gerrit.asterisk.org/6845">change 6845</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/6845"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: testsuite </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I67ec89e4a116526ad34796a233e15fec794cf3e1 </div>
<div style="display:none"> Gerrit-Change-Number: 6845 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 30 Oct 2017 16:16:23 +0000 </div>
<div style="display:none"> Gerrit-HasComments: No </div>