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

[Ashley Sanders (Code Review)]

Ashley Sanders has posted comments on this change.

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


Patch Set 1:

(6 comments)

Responses to most of the review feedback from Mark Michelson.

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):
> Unless I'm missing something, this, plus some of the other  on_* functions 
This function is what builds up the local container for channels created by this ari_client instance.

It is used as a result of the on_ws_event reading a message from the socket and parsing the message type to route that message to any local handler or any observer for the event (which, outside of the ari_client, there are no observers.)

For reference, look at line 253 in this file.

After reviewing this file, the only event handler that I found that was not used was on_channeldialplan. I will remove this function from the class for the next patch.


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][:]
> I'm not 100% sure on this, but since you are using [:], that will return a 
>From the python documentation https://docs.python.org/3/tutorial/datastructures.html#the-del-statement)

del some_list[:]

"clears the entire list [...] by assignment of an empty list to the slice"

This is equivalent to list.clear().


Line 108:             error += 'for event [{0}]; [Observers] is None.'.format(event)
> Nitpick: Start this string with a space, otherwise your error message will 
Correct. Good catch.


Line 132:         if self.__suspended < 0:
        :             self.__suspended = 0
> I don't imagine you actually want self.__suspended going less than 0, so th
Not including the = sign excludes this redundant case:
 
if some_variable = 0:
   some_variable = 0

If some_variable already equals 0, there is no need to reassign it.


Line 143:     def __validate(self, **kwargs):
> This method feels a bit over-defensive. It's only ever called from two plac
It is so that there are more meaningful messages in the logfile when debugging the tests. 

It does more than just scan the registrar for the event. It first determines if a parameter that was expected to have some value, actually has value. In the case of an event parameter, it scans the registrar for events matching the one provided. If this event is not registered, then it raises an error, with a more meaningful message such that debugging test writing is a bit easier.

The reason for having this in one place, rather than in the individual functions, is that this logic is used twice and anything used more than once deserves it's own function by virtue of the "Do not repeat yourself" principle.

http://stackoverflow.com/a/1749469
and
http://en.wikipedia.org/wiki/Don%27t_repeat_yourself


https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/run-test
File tests/rest_api/applications/stasisstatus/run-test:

Line 20: from stasisstatus.test_scenario_factory import build_scenarios
> Hm, I don't see test_scenario_factory in this diff. Is it missing from the 
Correct. This will be removed for the next patch.

FYI - The factory class was merged into run-test as a result of a code review issue from mjordan for the previous review. 

See: https://reviewboard.asterisk.org/r/4520/


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