[Asterisk-code-review] StatsD: Create a mock StatsD server in the Testsuite (testsuite[master])

[Ashley Sanders]

Ashley Sanders has posted comments on this change.

Change subject: StatsD: Create a mock StatsD server in the Testsuite
......................................................................


Patch Set 2: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/1313/2//COMMIT_MSG
Commit Message:

Line 9: Create a pluggable module that accepts configuration from a test-config.yaml
      : to indicate what messages are to be received from Asterisk. The mock server
      : is placed inside of the pluggable module and receives incoming messages.
      : The messages are then checked against the expected values.
This is a super nitnoid thing to complain about, but...

Usually, commit messages are not the same verbiage as your issue description, which is spoken in the same tense as a command, or something that has not yet been done.

For example, this would make more sense:

Created a pluggable module with a mock statsd server to provide a foundation for constructing tests for statsd dialplan application.


https://gerrit.asterisk.org/#/c/1313/2/tests/apps/statsd/nominal/mockd.py
File tests/apps/statsd/nominal/mockd.py:

Line 21: Protocol for the Mock Server to receive messages through
This sentence is odd - because it ends w/ a preposition, I thought at first it was an incomplete thought. Maybe state:

Protocol for the Mock Server to use for receiving messages.


Line 27: param mockd_server An instance of the mock StatsD server
This is missing, Keyword Arguments and we don't use param. 

For an example of how to construct your docstring for methods that contain arguments:

https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings


Line 73:     def __on_ami_connect(self, ami):
       :         ''' AMI connected handler
       : 
       :         :param The AMI instance that just connected
       :         '''
       :         LOGGER.debug('AMI instance %s Connected' % ami)
> This does nothing and should be removed.
I think the intent was to use the ami_connect observer to then start the server. 

In fact, I am not seeing where the listenUDP call is ever invoked in this test which begs the question, is this a working version of the code?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eaf92465b79569fff4db6ada16e8d215f7d214b
Gerrit-PatchSet: 2
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Tyler Cambron <tcambron at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes