[asterisk-dev] [Code Review] 4520: Testsuite: stasis: set a channel variable on websocket disconnect error

[Matt Jordan]

> On March 24, 2015, 11:23 a.m., Matt Jordan wrote:
> > ./asterisk/trunk/tests/rest_api/applications/stasis_status/test_case.py, lines 20-26
> > <https://reviewboard.asterisk.org/r/4520/diff/1/?file=72750#file72750line20>
> >
> >     You may want to consider moving this class back into runtest.
> >     
> >     While pulling other classes out of the runtest 'module' may be good, particularly since some of those could act as infrastructure for other tests, the typical approach in the testsuite is to put the class that orchestrates a test into that module.
> 
> Ashley Sanders wrote:
>     For this, I would argue that the test_case introduced by this test is actually a new, generalized variant of the base type of TestCase (the name needs to be modified a bit, though). 
>     
>     I think that it would make more sense to move the test_scenario_factory back into run-test, considering that is where all the specialized logic lives. 
>     
>     Thoughts?

I don't disagree that this *could* go in that direction. But for it to be truly generic, you'd want to:
(a) As you noted, change some of the naming and place it in lib/python/asterisk
(b) Have it be a test object in the pluggable framework, which would have to take some careful thought about how it could be configured to look at a particular set of scenarios. Generally, once we have a 'generic' piece of functionality, we'd want for it to be instantiated via the TestRunner, and then have either other pluggable modules attach into it, or have small Python modules provided by the test attach to it.

I do think having the scenario factory function get put into here would be fine. 

Overall, once you've got a reason to pull it out of run-test, I'm not against it - but I'm not sure this review is at that point.

The other direction you could take would be fully pull it out in this review, make it generic, and put it in lib/python/asterisk. But that might be more work than is necessary at this point, and since we don't have a second test (yet!) that would make use of it, I'm not sure it is warranted.


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4520/#review14783
-----------------------------------------------------------


On March 27, 2015, 10:58 a.m., Ashley Sanders wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4520/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 10:58 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24802
>     https://issues.asterisk.org/jira/browse/ASTERISK-24802
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis).
> 
> This patch introduces a new channel variable: STASISSTATUS to give outside applications context when errors occur in Stasis that interrupt normal processing.
> 
> This test exercises two scenarios to elicit updates to the STASISSTATUS channel variable:
> 1) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied.
> 2) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied.
> 
>  ***Note*** This is a test. It is only a test. The review for the Asterisk source can be found at: https://reviewboard.asterisk.org/r/4519/
> 
> 
> Diffs
> -----
> 
>   ./asterisk/trunk/tests/rest_api/applications/tests.yaml 6547 
>   ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test_scenario_factory.py PRE-CREATION 
>   ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test_scenario.py PRE-CREATION 
>   ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test_case.py PRE-CREATION 
>   ./asterisk/trunk/tests/rest_api/applications/stasisstatus/test-config.yaml PRE-CREATION 
>   ./asterisk/trunk/tests/rest_api/applications/stasisstatus/run-test PRE-CREATION 
>   ./asterisk/trunk/tests/rest_api/applications/stasisstatus/observable_object.py PRE-CREATION 
>   ./asterisk/trunk/tests/rest_api/applications/stasisstatus/monitor.py PRE-CREATION 
>   ./asterisk/trunk/tests/rest_api/applications/stasisstatus/configs/ast1/sip.conf PRE-CREATION 
>   ./asterisk/trunk/tests/rest_api/applications/stasisstatus/configs/ast1/extensions.conf PRE-CREATION 
>   ./asterisk/trunk/tests/rest_api/applications/stasisstatus/ari_client.py PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/4520/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ashley Sanders
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150327/683f2719/attachment.html>