<p> Attention is currently required from: Michael Bradeen. </p>
<p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/testsuite/+/16636">View Change</a></p><p>26 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Patchset:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636?tab=comments">Patch Set #3:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">I think for the most part the way you are testing things is fine, and feel free to leave as is.</p><p style="white-space: pre-wrap; word-wrap: break-word;">However, I think another option would be to reverse the calls, and have Asterisk originate them.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For the initial 3 valid cases the SIPp scenario(s) could then be simplified to only accept an invite. You could then use the same scenario file for all scenarios.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For the failure cases the 'Originate' should fail, which hopefully you can then check for the error(s), issue an UserEvent, check for at least that many user events using the 'EventActionModule' found in lib/python/asterisk/pluggable_modules.py, and then end the test.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Doing it this way should make it a little more deterministic (no waits), and also shorten execution time (again no waits).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File tests/rtp/rtp_range/rtp_range_even_even/sipp/call_accept_test_range.xml:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/dbb9604c_5a62b359">Patch Set #3, Line 87:</a> <code style="font-family:monospace,monospace">  </send>    </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove extra whitespace at eol.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/221f2f71_a863045e">Patch Set #3, Line 89:</a> <code style="font-family:monospace,monospace">  <label id="1"/> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove extra space at end</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/ef7dcae1_5f0e7956">Patch Set #3, Line 91:</a> <code style="font-family:monospace,monospace">  <send next="2">  </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove extra whitespace at eol.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/382d7394_090f4dcc">Patch Set #3, Line 107:</a> <code style="font-family:monospace,monospace">   </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove this whitespace too</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File tests/rtp/rtp_range/rtp_range_even_even/test-config.yaml:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/9011dc8e_db98c00c">Patch Set #3, Line 2:</a> <code style="font-family:monospace,monospace">inton</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/inton/into</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/f8249d49_5f5fe8e9">Patch Set #3, Line 4:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">        This test verifies that the correct RTP Range is used and that Asterisk does not go<br>        in to an infinite loop searching for a free RTP port<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd add a bit more in order to be more informational and separate it from the other tests. Maybe something like, "Using starting, and ending even values this test ..."</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File tests/rtp/rtp_range/rtp_range_even_odd/sipp/call_accept_test_range.xml:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/6bdd8bb4_427600a7">Patch Set #3, Line 87:</a> <code style="font-family:monospace,monospace">  </send>    </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/605df8be_a471e303">Patch Set #3, Line 89:</a> <code style="font-family:monospace,monospace">  <label id="1"/> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">extra space to remove</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/61fc5e4e_127d4b19">Patch Set #3, Line 91:</a> <code style="font-family:monospace,monospace">  <send next="2">  </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/fc60b18d_30583efa">Patch Set #3, Line 107:</a> <code style="font-family:monospace,monospace">   </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove whitespace</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File tests/rtp/rtp_range/rtp_range_even_odd/test-config.yaml:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/cb2b83cb_b3cef8cb">Patch Set #3, Line 2:</a> <code style="font-family:monospace,monospace">inton</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/inton/into</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/de3d1805_c82f9ca8">Patch Set #3, Line 4:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">        This test verifies that the correct RTP Range is used and that Asterisk does not go<br>        in to an infinite loop searching for a free RTP port<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd add a bit more in order to be more informational and separate it from the other tests. Maybe something like, "Using a starting even value, and an odd ending value this test ..."</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/f7210b09_7cfa9a97">Patch Set #3, Line 29:</a> <code style="font-family:monospace,monospace">        - RTP        </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File tests/rtp/rtp_range/rtp_range_odd_even/sipp/call_accept_test_range.xml:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/e6026d4a_c8d5f0dc">Patch Set #3, Line 87:</a> <code style="font-family:monospace,monospace">  </send>    </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/4eafd382_1368f48e">Patch Set #3, Line 89:</a> <code style="font-family:monospace,monospace">  <label id="1"/> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/64046bb9_8d692db4">Patch Set #3, Line 91:</a> <code style="font-family:monospace,monospace">  <send next="2">  </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/6ac1f9b5_354ced26">Patch Set #3, Line 107:</a> <code style="font-family:monospace,monospace">   </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File tests/rtp/rtp_range/rtp_range_odd_even/test-config.yaml:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/47f87256_426ef633">Patch Set #3, Line 2:</a> <code style="font-family:monospace,monospace">inton</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/inton/into</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/7aca99b4_cd860b1a">Patch Set #3, Line 4:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">        This test verifies that the correct RTP Range is used and that Asterisk does not go<br>        in to an infinite loop searching for a free RTP port<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd add a bit more in order to be more informational and separate it from the other tests. Maybe something like, "Using a starting odd, and an even ending value this test ..."</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File tests/rtp/rtp_range/rtp_range_odd_odd/sipp/call_accept_test_range.xml:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/bbdaf48d_639dd08b">Patch Set #3, Line 87:</a> <code style="font-family:monospace,monospace">  </send>    </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/bc0ffe08_cbcadb5d">Patch Set #3, Line 89:</a> <code style="font-family:monospace,monospace">  <label id="1"/> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/f063eeea_cf7a0cac">Patch Set #3, Line 91:</a> <code style="font-family:monospace,monospace">  <send next="2">  </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/da44d4ed_3893d82c">Patch Set #3, Line 107:</a> <code style="font-family:monospace,monospace">   </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File tests/rtp/rtp_range/rtp_range_odd_odd/test-config.yaml:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/8fc6175c_f137c6b8">Patch Set #3, Line 2:</a> <code style="font-family:monospace,monospace">inton</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/inton/into</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/testsuite/+/16636/comment/f6914d15_8a59bcf1">Patch Set #3, Line 4:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">        This test verifies that the correct RTP Range is used and that Asterisk does not go<br>        in to an infinite loop searching for a free RTP port<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd add a bit more in order to be more informational and separate it from the other tests. Maybe something like, "Using starting, and ending odd values this test ..."</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/testsuite/+/16636">change 16636</a>. To unsubscribe, or for help writing mail filters, 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/c/testsuite/+/16636"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: testsuite </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Iefa8545cc1785b1e32a42d740f8f567706c73199 </div>
<div style="display:none"> Gerrit-Change-Number: 16636 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Michael Bradeen <mbradeen@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Attention: Michael Bradeen <mbradeen@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 17 Nov 2021 22:02:34 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>