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

(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 <asanders at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes