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

(5 comments)

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

Line 45:         self.__ami.registerEvent('VarSet', self.__on_ami_varset)
       :         self.__ami.registerEvent('UserEvent', self.__on_ami_user_event)
       :         self.__ari_client.register_observers('on_stasisend',
       :                                              self.__on_stasisend)
       :         self.__ari_client.register_observers('on_stasisstart',
       :                                              self.__on_stasisstart)
       :         self.__ari_client.register_observers('on_channelvarset',
       :                                              self.__on_channelvarset)
> I'm not sure you need both AMI and ARI in this class.
You are right - AMI is all that I need for monitoring the channel variable.


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

Line 139:     def __validate(self, **kwargs):
I still need to apply the refactoring suggested by Mark Michelson in his review of patch 1.

Essentially, he pointed out that the way that this method is being used (as a means to validate that a given event has been registered in the registrar) does not require the flexibility of allowing a variable set of arguments to be supplied.


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

Line 87:     def __on_channelstatechange(self, sender, message):
This method has no usages and can safely be removed.


https://gerrit.asterisk.org/#/c/18/2/tests/rest_api/applications/stasisstatus/test-config.yaml
File tests/rest_api/applications/stasisstatus/test-config.yaml:

Line 31:         - ARI
       :         - Stasis
> Stasis is, in many ways, synonymous currently with ARI. Since we already do
Noted.


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

Line 37:         TestCase.__init__(self)
I noticed while reviewing Jonathan's review, https://gerrit.asterisk.org/#/c/28/, that pylint had been lying to me. TestCase is not an older-style class and all calls to the base type's methods should be invoked using the super keyword. There are several places in this file that need to be updated accordingly:
ln 37 (here), ln 64, ln 126, and ln 142.


-- 
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: 2
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-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes