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

[Matt Jordan]

Matt Jordan has posted comments on this change.

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


Patch Set 2: Code-Review-1

(5 comments)

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

Line 15: sys.path.append("tests/apps/statsd/nominal")
Why are you appending this to the Python path? You should only need to do this if you are importing a library from this path.


Line 43:     ''' Pluggable Module object that acts as a StatsD server to examine
Super nitpick: Summaries should fit on a single line.

Suggestion:

"""Pluggable Module that acts as a mock StatsD server
"""


Line 55:         self.test_object.register_ami_observer(self.__on_ami_connect)
Nitpick: you don't need to overly hide your AMI connection handler, as you probably don't expect this class to be sub-classed in the first place. I'd go with a single '_'.


Line 67:         if self.config[0] == self.message:
Why does this only test a single packet?

I would restructure this to do the following:
(1) On each message received, append the received message to a list.
(2) When the number of messages received matches the config:
    - Stop the test. This should trigger the logic in your stop observer.
(3) In your stop observer:
    - If the number of received messages does not match the config, fail the test
    - If the number of received messages does match the config, verify each packet
    - If any packet doesn't verify, fail
    - If all packets verify, success


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.


-- 
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: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes