Mark Michelson has posted comments on this change. Change subject: stasis: set a channel variable on websocket disconnect error ......................................................................
Patch Set 1: (10 comments) 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 below, don't appear to be called during these tests. Can they be removed? 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 copy of the list at self.__registrar[event] rather than a reference to the actual list, meaning that you aren't actually deleting the list at self.__registrar[event] I think that del self.__registrar[event] is actually what's wanted here. Line 108: error += 'for event [{0}]; [Observers] is None.'.format(event) Nitpick: Start this string with a space, otherwise your error message will have "Could not register observersfor event..." Line 122: if self.__registrar[event] is None: : msg += 'Instantiating the observers for event {0}.'.format(event) : LOGGER.debug(msg) : self.__registrar[event] = list() I may be misinterpreting your intent here, but I don't think this is going to work how you expect it to. I've interpreted this block of code to mean that if self.__registrar does not currently have the event key within it, then this corrects the problem by setting self.__registrars[event] to be an empty list. The problem with this is that if the key does not exist in self.__registrars, then the if statement will throw a KeyError, not return None. You would need to go with: if event not in self.__registrar: instead. Now, if you actually are trying to see if the value stored at the event key in self.__registrar is None, then feel free to ignore this finding completely. Line 132: if self.__suspended < 0: : self.__suspended = 0 I don't imagine you actually want self.__suspended going less than 0, so this should probably be <= instead of < Line 143: def __validate(self, **kwargs): This method feels a bit over-defensive. It's only ever called from two places, and it has a single string passed to it. The method could be reduced down to def __validate(self, event): return True if event in self.__registrar else False 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 diff? https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/test_case.py File tests/rest_api/applications/stasisstatus/test_case.py: Line 64: TestCase.ami_connect(self, ami) Calling TestCase.ami_connect() isn't necessary since ami_connect is a virtual method in the first place. It's designed for you to just do what you need to do for your derived class and be on your way. Line 126: TestCase.run(self) : : self.create_ami_factory() I think that TestCase.run(self) will already create the AMI factory, so you don't need to do it yourself here. In fact, I don't think it's really necessary to override this method since It's just doing the same thing as the parent (plus printing a debug message) Line 142: TestCase.stop_reactor(self) This notation is a bit odd. Why not just self.stop_reactor()? -- 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: 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