[asterisk-dev] Change in testsuite[master]: stasis: set a channel variable on websocket disconnect error

[Mark Michelson (Code Review)]

Mark Michelson has posted comments on this change.

Change subject: stasis: set a channel variable on websocket disconnect error
......................................................................


Patch Set 1: Code-Review+1

(4 comments)

I noticed that I gave the Code-Review a 0 last time. I figure that a +1 is actually in order since I noticed that my comments last time weren't about functional deficiencies so much as style and nitpicks. So here's my +1.

https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/ari_client.py
File tests/rest_api/applications/stasisstatus/ari_client.py:

Line 148:     def on_channelcreated(self, message):
> This function is what builds up the local container for channels created by
Ah, I missed the getattr() call in on_ws_event(). I'm going to blame gerrit's hijacking of my browser's search :-P


https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/observable_object.py
File tests/rest_api/applications/stasisstatus/observable_object.py:

Line 84:             del self.__registrar[event][:]
> From the python documentation https://docs.python.org/3/tutorial/datastruct
Ah, thanks for the enlightenment!


Line 132:         if self.__suspended < 0:
        :             self.__suspended = 0
> Not including the = sign excludes this redundant case:
Correct, but my point was that if self.__suspended is 0 when entering this function, the result will be that it ends up being set to -1. My assumption was that you wanted self.__suspended to have a lower bound of 0. My suggestion was made in the spirit of brevity, but you could just as easily have

if self.__suspended == 0:
    return
elif self.__suspended > 0:
    self.__suspended -= 1
else:
    # WTF, how did it get to be < 0?


Line 143:     def __validate(self, **kwargs):
> It is so that there are more meaningful messages in the logfile when debugg
I wasn't suggesting getting rid of the method, just rewriting its body.

While DRY is fantastic, I'm also a firm believer in YAGNI, especially for tests that are concentrated on a specific case, where the data that is to be used is self-generated and known in advance. In this case, you've written a generic validation function that can take any number of named parameters, and based on the name of the parameter, perform some specific action. In actual use, the only parameter ever passed to this function is a single parameter called "event" (never multiple parameters), and it will never be NoneType (since the data being passed to this function is generated by your own test cases), and I don't actually think that any of the operations you perform could throw an IndexError anway. There's no reason to build a trans-oceanic cruise liner to cross a creek.

The error message argument makes sense. If you want to keep an error message in here, that's cool.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f7dadfd429bd30e9f07a531f47884d8c923fc13
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes