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

Addressed Matt Jordan's review feedback.

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

Line 71:         while not self.clean:
       :             LOGGER.debug('{0} I\'m not so fresh so clean.'.format(self))
       :             time.sleep(1)
> There's a few problems with this particular approach.
I opted for option #2. In the case that we get stuck in a loop, the reactor will timeout, causing the test to fail. I think this is the preferred behavior in that the reactor is handling the timing and it will assuredly blow up quickly if something gets stuck during processing.


Line 175:     def on_channelstatechange(self, message):
While going through Matt's review feedback, I realized the only place this was being used was from a method that was never called in BabsTestScenario.


Line 270:         resource              --  The resource to use to construct the endpoint
        :                                   for the ARI request. (optional)
        :                                   (default None.) If no value is provided,
        :                                   resource will automatically be generated in
        :                                   the form: 'LOCAL/<app_name>@Acme'.
> Good catch. Yes, I 100% agree with you. This should be refactored to live i
Also, since it is not mandatory for a client to pass an app parameter to ARI, I added a check for that argument and if it is None, it does not include it in the post to ARI.


https://gerrit.asterisk.org/#/c/18/2/tests/rest_api/applications/stasisstatus/configs/ast1/extensions.conf
File tests/rest_api/applications/stasisstatus/configs/ast1/extensions.conf:

Line 4: 
I am missing the Babs extension in this file.


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 33:         - Application
       :         - STASISSTATUS
> I'd remove these two tags.
Noted. I reviewed all the current tags in the testsuite and the only ones that are applicable to this are ARI and Application. I also added a tag for the jira issue.


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