[Asterisk-code-review] sipp, test suite utils: Default media port to an unused port (testsuite[13])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/8689 )

Change subject: sipp, test_suite_utils: Default media port to an unused port
......................................................................


Patch Set 6: Code-Review-1

(7 comments)

The -1 is for the sipp.py comment only.  All other findings can be deferred if you want.

https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/self_test/test_utils_socket.py
File lib/python/asterisk/self_test/test_utils_socket.py:

PS6: 
pep8 produces many warnings:
E731 do not assign a lambda expression, use a def

If you want to fix it great if not that's fine we can get it later.


https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/self_test/test_utils_socket.py@14
PS6, Line 14: import os
os is unused in this source.


https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/self_test/test_utils_socket.py@21
PS6, Line 21: sys.path.append(os.path.dirname(os.path.dirname(os.path.realpath(__file__))))
            : from utils_socket import Ports, PortError, get_available_port, MIN_PORT
The rest of the tests I'm working on use code as follows:
sys.path.append('lib/python')  # noqa

from asterisk.utils_socket import Ports, PortError, get_available_port, MIN_PORT


Technically nothing wrong with what you've done here although it is confusing because the 2 os.path.dirname actually adds lib/python/asterisk to sys.path.

The '  # noqa' (two spaces before hash) cause that line to be ignored by pep8.  This suppresses a warning about imports being


https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/sipp.py
File lib/python/asterisk/sipp.py:

https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/sipp.py@685
PS6, Line 685:                 config=default_args, num=5))
Do we actually need 5 ports?  I thought we only needed 4:
audio rtp
audio rtcp
video rtp
video rtcp

What is the 5th port used for?


https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/utils_socket.py
File lib/python/asterisk/utils_socket.py:

https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/utils_socket.py@1
PS6, Line 1: #!/usr/bin/env python
Please remove this for clarity that it is not an executable script.


https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/utils_socket.py@14
PS6, Line 14: import sys
Unused import


https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/utils_socket.py@192
PS6, Line 192:         if not isinstance(ports, list):
             :             ports = [ports]
This can be made compatible with python3 by:
if isinstance(ports, int):
    ports = [ports]

if not isinstance(ports, list):
    ports = list(ports)

In python3 it is an error if isinstance(ports, range), but this cannot be tested directly in python2.  I'm unsure this may be an issue for Ports.reserve() as well, that is not hit by the unit tests.



-- 
To view, visit https://gerrit.asterisk.org/8689
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: testsuite
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I3da461123afc30e1f5ca12e65d289eaa42d6de00
Gerrit-Change-Number: 8689
Gerrit-PatchSet: 6
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Thu, 19 Apr 2018 20:32:03 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180419/3bba42ec/attachment-0001.html>