[Asterisk-code-review] testsuite: add tests to ensure ice in ANSWER only if offered (testsuite[master])
Kevin Harwell
asteriskteam at digium.com
Wed Jul 18 12:04:51 CDT 2018
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/9428 )
Change subject: testsuite: add tests to ensure ice in ANSWER only if offered
......................................................................
Patch Set 2: Code-Review-1
(26 comments)
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:
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/configs/ast1/extensions.conf@11
PS2, Line 11: exten => _X.,1,Dial(pjsip/sbc,180)
Instead of dialing another scenario I think for this test you can just answer here and drop the party B scenario altogether?
exten => answer,1,NoOp()
same => n,Answer()
same => n,Hangup()
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:
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@1
PS2, Line 1: #!/usr/bin/env python
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.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@3
PS2, Line 3: Copyright (C) 2018, Voxbone, SA
delete the extra space at the end of the line.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@12
PS2, Line 12: import logging
: import signal
: import subprocess
: import time
remove unused imports
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@26
PS2, Line 26: logger = logging.getLogger(__name__)
'logger' appears unused, so can be removed.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@28
PS2, Line 28: sippA_logfile = WORKING_DIR + "/A_PARTY.log"
: sippA_errfile = WORKING_DIR + "/A_PARTY_ERR.log"
: sippB_logfile = WORKING_DIR + "/B_PARTY.log"
: sippB_errfile = WORKING_DIR + "/B_PARTY_ERR.log"
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).
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@38
PS2, Line 38: '-message_file' : sippB_logfile,
: '-error_file' : sippB_errfile,
: '-trace_msg' : '-trace_err',
logging can be removed.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@34
PS2, Line 34: {
: 'scenario' : 'B_PARTY.xml',
: '-i' : '127.0.0.1',
: '-p' : '5700',
: '-message_file' : sippB_logfile,
: '-error_file' : sippB_errfile,
: '-trace_msg' : '-trace_err',
: },
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.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@47
PS2, Line 47: '-message_file' : sippA_logfile,
: '-error_file' : sippA_errfile,
: '-trace_msg' : '-trace_err',
logging stuff can be removed.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/run-test@55
PS2, Line 55: test.reactor_timeout = 65;
Is there a reason for the raised reactor_timeout? If not can probably be safely removed and just used the default (30 seconds)
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:
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/sipp/A_PARTY.xml@62
PS2, Line 62: <log message="Log to avoid the problem of not using $1 [$1]"/>
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:
<Reference variables="1" />
See 'tests/channels/pjsip/set_var/sipp/invite_recv.xml' for example use.
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:
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/sipp/B_PARTY.xml@72
PS2, Line 72:
remove extra space/red blob
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:
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/test-config.yaml@8
PS2, Line 8: dependencies:
: - app : 'sipp'
add the following as a dependency:
- asterisk : 'res_pjsip'
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_not_offered/test-config.yaml@11
PS2, Line 11: - SIP
tag should be 'pjsip'
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test
File tests/channels/pjsip/ice/ice_offered/run-test:
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@1
PS2, Line 1: #!/usr/bin/env python
Same as other test:
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.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@12
PS2, Line 12: import logging
: import signal
: import subprocess
: import time
remove unused imports
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@26
PS2, Line 26: logger = logging.getLogger(__name__)
'logger' is unused, so remove.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@28
PS2, Line 28: sippA_logfile = WORKING_DIR + "/A_PARTY.log"
: sippA_errfile = WORKING_DIR + "/A_PARTY_ERR.log"
: sippB_logfile = WORKING_DIR + "/B_PARTY.log"
: sippB_errfile = WORKING_DIR + "/B_PARTY_ERR.log"
Same as the other test. Drop the sipp logging stuff.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@38
PS2, Line 38: '-message_file' : sippB_logfile,
: '-error_file' : sippB_errfile,
: '-trace_msg' : '-trace_err',
remove logging.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@34
PS2, Line 34: {
: 'scenario' : 'B_PARTY.xml',
: '-i' : '127.0.0.1',
: '-p' : '5700',
: '-message_file' : sippB_logfile,
: '-error_file' : sippB_errfile,
: '-trace_msg' : '-trace_err',
: },
Same as other test. I believe this scenario can be completely dropped since the negotiation is tested between Asterisk and party A.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@47
PS2, Line 47: '-message_file' : sippA_logfile,
: '-error_file' : sippA_errfile,
: '-trace_msg' : '-trace_err',
remove logging.
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/run-test@55
PS2, Line 55: test.reactor_timeout = 65;
If no reason for the increased time out then drop.
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:
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/sipp/A_PARTY.xml@69
PS2, Line 69: <log message="Log to avoid the problem of not using $1 [$1]"/>
Same as other one. Drop the log message and use the 'Reference' tag:
<Reference variables="1" />
See 'tests/channels/pjsip/set_var/sipp/invite_recv.xml' for example use.
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:
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/sipp/B_PARTY.xml@72
PS2, Line 72:
Delete extra space.
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:
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/test-config.yaml@8
PS2, Line 8: dependencies:
add the following as a dependency:
- asterisk : 'res_pjsip'
https://gerrit.asterisk.org/#/c/9428/2/tests/channels/pjsip/ice/ice_offered/test-config.yaml@11
PS2, Line 11: - SIP
s/SIP/pjsip
--
To view, visit https://gerrit.asterisk.org/9428
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4744c062e17862bf60eaa76f46af2ba2743e650
Gerrit-Change-Number: 9428
Gerrit-PatchSet: 2
Gerrit-Owner: Torrey Searle <tsearle at gmail.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Wed, 18 Jul 2018 17:04:51 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180718/1564d5f4/attachment-0001.html>
More information about the asterisk-code-review
mailing list