[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 3: Code-Review-1

(4 comments)

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

Line 68:         self.message = message
Don't assign message to a member variable.

You're already recording each message in a member variable, the list self.packets. self.message is redundant (and useless, as you should be only inspecting all packets, and not just the last received one).


Line 81:         if len(self.packets) == len(self.config):
Two issues here:

1) Invert the logic so that you can reduce indentation. That is, test to see if the lengths are NOT equal and fail the test/return, then continue on.

2) You should fail the test if self.packets is not the same length as the configuration packets:

if len(self.packet) != len(self.config):
    LOGGER.warn('Number of received packets {0} is not equal to the number of configured packets {1}'.format(len(self.packets), len(self.config)))
    self.test_object.set_passed(False)
    return

...


Line 83:                 LOGGER.debug(packet)
Don't log out the packet here. You already log the packet out when you receive it; logging it again here provides no additional value.


Line 84:                 if packet not in self.config:
This assumes that order doesn't matter, which is probably not correct. You should be comparing each packet that was received to each packet that was expected to be received, in the same order.

Since order matters, we can't just turn these into sets, nor should we be using the 'in' operator. Instead, we can use the zip built-in function with a list comprehension to give us back which packets didn't match:

failed_matches = [actual, expected for actual, expected in zip(self.packets, self.config) if actual != expected]
if len(failed_matches) != 0:
    LOGGER.warn('The following packets failed to match: {0}'.format(failed_matches))
    self.test_object.set_passed(False)
    return

LOGGER.info('All packets matched')
self.test_object.set_passed(True)


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