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

[Ashley Sanders]

Ashley Sanders has posted comments on this change.

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


Patch Set 6:

(12 comments)

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 session
Done


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 ta
Done


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
Done


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


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 dist
Done


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


Line 181:             return (
        :                 ConnectionState.DONE if se.args[0] == EWOULDBLOCK
        :                 else ConnectionState.LOST)
> Okay, then I would suggest you add the part about how you know this state i
Done


Line 253:         Overrides twisted.internet.protocol.ClientFactory.clientConnectionFailed
> 80 characters (79 is the limit)... and I'm not sure of a good way to shorte
Done


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


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


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.
Done


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


-- 
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