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 <asand...@digium.com> Gerrit-Reviewer: Ashley Sanders <asand...@digium.com> Gerrit-Reviewer: Mark Michelson <mmichel...@digium.com> Gerrit-HasComments: Yes -- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
