<p>Corey Farrell <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;"><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?</p><p style="white-space: pre-wrap; word-wrap: break-word;">--reactor-timeout-adjustment=5 to add 5 seconds, or if you have a<br>really fast machine,  --reactor-timeout-adjustment=-5 to reduce 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 timeout is a side-effect, not the main point.  Reactor timeout is only touched to deal with the case where a test needs >30 second shutdown delay.  So if we have memcheck-delay-stop: 90 to deal with a minse timer we increase the reactor-timeout to 93 seconds minimum to avoid showing reactor timeout messages while the test is stopping.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This patch is not intended to deal with any slowness caused by valgrind or REF_DEBUG (code to do that globally is already in place).  The point is to wait long enough for any relevant timers to fire and cleanup resources.  SIP timer_b takes just as long to fire with or without memory checking, the only difference is that 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 about half the PJSIP tests and is unneeded by nearly all non-PJSIP tests.  Many of the PJSIP tests will need 7 seconds (assuming timer_b is configured to minimum).  Some tests will need 60 seconds (ast_sip_session_defer_termination), others might need 90 seconds (minse configured to minimum).  I had considered setting the default memcheck-delay-stop to 7 seconds but this would have drawbacks.  A 7 second delay for every test would increase the run-time of the full testsuite by nearly 2 hours (1000 tests).  Maybe 200 of the tests need any delay.  Most cases we're dealing with timer_b which means that we need to reconfigured timer_t1 + timer_b or use memcheck-delay-stop: 32, so for a 7 second delay to ever be effective we'd need to touch the pjsip.conf files anyways.</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 15:10:07 +0000 </div>
<div style="display:none"> Gerrit-HasComments: No </div>