[Asterisk-code-review] SIP: Rewrite tcpauthlimit test in Python (testsuite[master])

[Jonathan Rose]

Jonathan Rose has posted comments on this change.

Change subject: SIP: Rewrite tcpauthlimit test in Python
......................................................................


Patch Set 6:

(11 comments)

Sorry, I couldn't help myself but to go ahead and run a linter. There is also a minor finding about test descriptions, but all in all none of this is very important.

https://gerrit.asterisk.org/#/c/1055/6/tests/channels/SIP/tcpauthlimit/sipp_client_scenario/test-config.yaml
File tests/channels/SIP/tcpauthlimit/sipp_client_scenario/test-config.yaml:

Line 4:         This test ensures that chan_sip respects the tcpauthlimit config
      :         option by running the following scenario:
      :             * The SIPpScenario: attempt to create n*2 SIPp processes, where n
      :         is the value of the 'tcpauthlimit' property in sip.conf. Each SIPp
      :         scenario is configured to connect to the same Asterisk host. If the
      :         'tcpauthlimit' property is honored, only n of these scenarios will
      :         pass, while the remaining n will fail.
I feel like this description should mention something about how the sessions are unauthenticated.


https://gerrit.asterisk.org/#/c/1055/6/tests/channels/SIP/tcpauthlimit/sipp_scenario.py
File tests/channels/SIP/tcpauthlimit/sipp_scenario.py:

Line 19:     """Wrapper class for a SIPpScenario.
       : 
       :     This class provides the ability to override the default results evaluation
       :     mechanism of the SIPpScenario as specified in the test module configuration.
one tiny little character over the line length standard on the last line tagged here.


https://gerrit.asterisk.org/#/c/1055/6/tests/channels/SIP/tcpauthlimit/tcp_scenario.py
File tests/channels/SIP/tcpauthlimit/tcp_scenario.py:

Line 19: LOGGER = logging.getLogger(__name__)
       : 
       : class ConnectionState(object):
       :     """An enumeration to describe the connection state of a client."""
add another blank line


Line 48:         return 'UNKNOWN'
       : 
       : class TcpClient(LineReceiver):
add another blank line


Line 83:         if (self.__connection_state == ConnectionState.CONNECTING or
       :             self.__connection_state == ConnectionState.CONNECTED):
       :             self.__connection_state = ConnectionState.LOST
Violates style for line continuation. Linter wants you to make line 84 distinguished from 85.


Line 122:         In the event that the connector fails and we don't get the notification,
80 characters (79 is the limit)


Line 253:         Overrides twisted.internet.protocol.ClientFactory.clientConnectionFailed
80 characters (79 is the limit)... and I'm not sure of a good way to shorten this one. Maybe ignore it.


Line 571:         msg = '{0} Received a connection change notification from client [{1}].'
80 characters (79 is the limit)


Line 580:             msg += '\tBased on the test configuration, I expected to receive:\n'
80 characters (79 is the limit)


https://gerrit.asterisk.org/#/c/1055/6/tests/channels/SIP/tcpauthlimit/tcpauthlimit.py
File tests/channels/SIP/tcpauthlimit/tcpauthlimit.py:

Line 19: LOGGER = logging.getLogger(__name__)
       : 
       : def get_friendly_scenario_type(scenario_type):
       :     """Returns the logger friendly name for the given scenario type.
Should have two blank lines between these.


Line 34:     return 'Unknown type scenario'
       : 
       : class TcpAuthLimitTestModule(object):
two blank lines here as well.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica28ba0ca7ae92b3546da4cd23458f289c111d36
Gerrit-PatchSet: 6
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Jonathan Rose <jrose at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes