<p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/9428">View Change</a></p><p>26 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/configs/ast1/extensions.conf">File tests/channels/pjsip/ice/ice_not_offered/configs/ast1/extensions.conf:</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/9428/2/tests/channels/pjsip/ice/ice_not_offered/configs/ast1/extensions.conf@11">Patch Set #2, Line 11:</a> <code style="font-family:monospace,monospace">exten => _X.,1,Dial(pjsip/sbc,180)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Instead of dialing another scenario I think for this test you can just answer here and drop the party B scenario altogether?</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">exten => answer,1,NoOp()<br> same => n,Answer()<br> same => n,Hangup()</pre></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test">File tests/channels/pjsip/ice/ice_not_offered/run-test:</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/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@1">Patch Set #2, Line 1:</a> <code style="font-family:monospace,monospace">#!/usr/bin/env python</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This test can be setup/initiated strictly from the test-config.yaml file if you like by using the 'sipp.SIPpTestCase' as you test object in the yaml. See 'tests/channels/pjsip/sdp_offer_answer/incoming/nominal/multiple-media-stream/audio/accept/' for an example.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@3">Patch Set #2, Line 3:</a> <code style="font-family:monospace,monospace">Copyright (C) 2018, Voxbone, SA </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">delete the extra space at the end of the line.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@12">Patch Set #2, Line 12:</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;">import logging<br>import signal<br>import subprocess<br>import time<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove unused imports</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@26">Patch Set #2, Line 26:</a> <code style="font-family:monospace,monospace">logger = logging.getLogger(__name__)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">'logger' appears unused, so can be removed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@28">Patch Set #2, Line 28:</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;">sippA_logfile = WORKING_DIR + "/A_PARTY.log"<br>sippA_errfile = WORKING_DIR + "/A_PARTY_ERR.log"<br>sippB_logfile = WORKING_DIR + "/B_PARTY.log"<br>sippB_errfile = WORKING_DIR + "/B_PARTY_ERR.log"<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It appears the sipp logging and error logging is not really needed for this test and can/should probably be removed. If the test fails it should be obvious what happened (regex matched when it shouldn't).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@38">Patch Set #2, Line 38:</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;">        '-message_file' : sippB_logfile,<br>        '-error_file' : sippB_errfile,<br>        '-trace_msg' : '-trace_err',<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">logging can be removed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@34">Patch Set #2, Line 34:</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;">   {<br>        'scenario' : 'B_PARTY.xml',<br>        '-i' : '127.0.0.1',<br>        '-p' : '5700',<br>        '-message_file' : sippB_logfile,<br>        '-error_file' : sippB_errfile,<br>        '-trace_msg' : '-trace_err',<br>    },<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It appears you are just testing the negotiation between party A and Asterisk (making sure Asterisk doesn't send back ice candidates), so I think it'd be okay to drop Party B altogether from this test and in extensions.conf just make it so Asterisk answers instead of dialing.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@47">Patch Set #2, Line 47:</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;">        '-message_file' : sippA_logfile,<br>        '-error_file' : sippA_errfile,<br>        '-trace_msg' : '-trace_err',<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">logging stuff can be removed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@55">Patch Set #2, Line 55:</a> <code style="font-family:monospace,monospace">    test.reactor_timeout = 65;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is there a reason for the raised reactor_timeout? If not can probably be safely removed and just used the default (30 seconds)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/sipp/A_PARTY.xml">File tests/channels/pjsip/ice/ice_not_offered/sipp/A_PARTY.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/9428/2/tests/channels/pjsip/ice/ice_not_offered/sipp/A_PARTY.xml@62">Patch Set #2, Line 62:</a> <code style="font-family:monospace,monospace">        <log message="Log to avoid the problem of not using $1 [$1]"/></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It appears you're just logging in order to reference the variable (as sipp will error out if unused). However you can instead use the 'Reference' tag and it should avoid this problem:</p><p style="white-space: pre-wrap; word-wrap: break-word;"><Reference variables="1" /></p><p style="white-space: pre-wrap; word-wrap: break-word;">See 'tests/channels/pjsip/set_var/sipp/invite_recv.xml' for example use.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/sipp/B_PARTY.xml">File tests/channels/pjsip/ice/ice_not_offered/sipp/B_PARTY.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/9428/2/tests/channels/pjsip/ice/ice_not_offered/sipp/B_PARTY.xml@72">Patch Set #2, Line 72:</a> <code style="font-family:monospace,monospace"> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove extra space/red blob</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/test-config.yaml">File tests/channels/pjsip/ice/ice_not_offered/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/9428/2/tests/channels/pjsip/ice/ice_not_offered/test-config.yaml@8">Patch Set #2, Line 8:</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;">    dependencies:<br>        - app : 'sipp'<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">add the following as a dependency:</p><ul><li>asterisk : 'res_pjsip'</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/test-config.yaml@11">Patch Set #2, Line 11:</a> <code style="font-family:monospace,monospace">        - SIP</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">tag should be 'pjsip'</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test">File tests/channels/pjsip/ice/ice_offered/run-test:</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/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@1">Patch Set #2, Line 1:</a> <code style="font-family:monospace,monospace">#!/usr/bin/env python</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same as other test:</p><p style="white-space: pre-wrap; word-wrap: break-word;">This test can be setup/initiated strictly from the test-config.yaml file if you like by using the 'sipp.SIPpTestCase' as you test object in the yaml. See 'tests/channels/pjsip/sdp_offer_answer/incoming/nominal/multiple-media-stream/audio/accept/' for an example.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@12">Patch Set #2, Line 12:</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;">import logging<br>import signal<br>import subprocess<br>import time<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove unused imports</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@26">Patch Set #2, Line 26:</a> <code style="font-family:monospace,monospace">logger = logging.getLogger(__name__)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">'logger' is unused, so remove.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@28">Patch Set #2, Line 28:</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;">sippA_logfile = WORKING_DIR + "/A_PARTY.log"<br>sippA_errfile = WORKING_DIR + "/A_PARTY_ERR.log"<br>sippB_logfile = WORKING_DIR + "/B_PARTY.log"<br>sippB_errfile = WORKING_DIR + "/B_PARTY_ERR.log"<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same as the other test. Drop the sipp logging stuff.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@38">Patch Set #2, Line 38:</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;">        '-message_file' : sippB_logfile,<br>        '-error_file' : sippB_errfile,<br>        '-trace_msg' : '-trace_err',<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove logging.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@34">Patch Set #2, Line 34:</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;">    {<br>        'scenario' : 'B_PARTY.xml',<br>        '-i' : '127.0.0.1',<br>        '-p' : '5700',<br>        '-message_file' : sippB_logfile,<br>        '-error_file' : sippB_errfile,<br>        '-trace_msg' : '-trace_err',<br>    },<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same as other test. I believe this scenario can be completely dropped since the negotiation is tested between Asterisk and party A.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@47">Patch Set #2, Line 47:</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;">        '-message_file' : sippA_logfile,<br>        '-error_file' : sippA_errfile,<br>        '-trace_msg' : '-trace_err',<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove logging.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@55">Patch Set #2, Line 55:</a> <code style="font-family:monospace,monospace">    test.reactor_timeout = 65;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If no reason for the increased time out then drop.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/sipp/A_PARTY.xml">File tests/channels/pjsip/ice/ice_offered/sipp/A_PARTY.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/9428/2/tests/channels/pjsip/ice/ice_offered/sipp/A_PARTY.xml@69">Patch Set #2, Line 69:</a> <code style="font-family:monospace,monospace">        <log message="Log to avoid the problem of not using $1 [$1]"/></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same as other one. Drop the log message and use the 'Reference' tag:</p><p style="white-space: pre-wrap; word-wrap: break-word;"><Reference variables="1" /></p><p style="white-space: pre-wrap; word-wrap: break-word;">See 'tests/channels/pjsip/set_var/sipp/invite_recv.xml' for example use.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/sipp/B_PARTY.xml">File tests/channels/pjsip/ice/ice_offered/sipp/B_PARTY.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/9428/2/tests/channels/pjsip/ice/ice_offered/sipp/B_PARTY.xml@72">Patch Set #2, Line 72:</a> <code style="font-family:monospace,monospace"> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Delete extra space.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/test-config.yaml">File tests/channels/pjsip/ice/ice_offered/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/9428/2/tests/channels/pjsip/ice/ice_offered/test-config.yaml@8">Patch Set #2, Line 8:</a> <code style="font-family:monospace,monospace">    dependencies:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">add the following as a dependency:</p><ul><li>asterisk : 'res_pjsip'</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/test-config.yaml@11">Patch Set #2, Line 11:</a> <code style="font-family:monospace,monospace">        - SIP</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/SIP/pjsip</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9428">change 9428</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/9428"/><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: Ic4744c062e17862bf60eaa76f46af2ba2743e650 </div>
<div style="display:none"> Gerrit-Change-Number: 9428 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Torrey Searle <tsearle@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 18 Jul 2018 17:04:51 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>