[asterisk-dev] [Code Review] 4520: Testsuite: stasis: set a channel variable on websocket disconnect error
Matt Jordan
reviewboard at asterisk.org
Tue Mar 24 11:23:42 CDT 2015
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4520/#review14783
-----------------------------------------------------------
./asterisk/trunk/tests/rest_api/applications/stasis_status/configs/ast1/ari.conf
<https://reviewboard.asterisk.org/r/4520/#comment25368>
I'd use the default ari.conf provided by the testsuite, since there aren't any parameters in it that you need to override.
That does mean that instead of sherman/wabac, you'll need to use a user/pass of testsuite/testsuite.
./asterisk/trunk/tests/rest_api/applications/stasis_status/configs/ast1/extensions.conf
<https://reviewboard.asterisk.org/r/4520/#comment25367>
Parentheses are unbalanced:
same => n,GoSub(subComicDispenser,1(Ignore))
This applies to all but the [Bb]uster extension on.
./asterisk/trunk/tests/rest_api/applications/stasis_status/configs/ast1/http.conf
<https://reviewboard.asterisk.org/r/4520/#comment25366>
You shouldn't need http.conf, as the default (in configs/http.conf) should be sufficient.
./asterisk/trunk/tests/rest_api/applications/stasis_status/observable_object.py
<https://reviewboard.asterisk.org/r/4520/#comment25387>
Rather than injecting a name down into the base class, how about having a method on the class that derived classes should override to provide the name?
This would let you do something like:
class ObservableObject(object):
def get_name(self):
return ''
def __format__(self, format_spec):
return self.__class__.__name__ + '[' + self.get_name() + ']'
class ConcreteObserver(ObservableObject):
def get_name(self):
return self.__class__.__name__
./asterisk/trunk/tests/rest_api/applications/stasis_status/observable_object.py
<https://reviewboard.asterisk.org/r/4520/#comment25388>
When you have 'ternary-esque' expressions (this thing is a Boolean, unless it is None), I find it clearer to write the if statement as a one-liner:
return valid if valid is not None else True
./asterisk/trunk/tests/rest_api/applications/stasis_status/observable_object.py
<https://reviewboard.asterisk.org/r/4520/#comment25389>
Should an ObservableObject raise an exception in this case?
This feels like it would be a test writing error, which means I would expect things to blow up (so that, as a test writer, I can fix them). Right now, if this just returns, test execution will hum along without actually indicating (other than as a WARNING) that something really bad just happened.
This finding applies to the other very off nominal paths in this class, particularly those where methods are passed invalid parameters.
./asterisk/trunk/tests/rest_api/applications/stasis_status/run-test
<https://reviewboard.asterisk.org/r/4520/#comment25370>
Since the StasisStatusTestCase is located in the local test_case module, you don't need either of these variables.
./asterisk/trunk/tests/rest_api/applications/stasis_status/test_case.py
<https://reviewboard.asterisk.org/r/4520/#comment25390>
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.
./asterisk/trunk/tests/rest_api/applications/stasis_status/test_case.py
<https://reviewboard.asterisk.org/r/4520/#comment25369>
Change the credentials to 'testsuite'/'testsuite' to use the default ari.conf provided by the Testsuite.
./asterisk/trunk/tests/rest_api/applications/stasis_status/test_scenario.py
<https://reviewboard.asterisk.org/r/4520/#comment25377>
Since this isn't really an event handler, I'd rename it to 'finish_strategy' or something similar.
Generally, this is always called by concrete implementations of the TestScenario, as opposed to something that this has subscribed to.
./asterisk/trunk/tests/rest_api/applications/stasis_status/test_scenario.py
<https://reviewboard.asterisk.org/r/4520/#comment25375>
I think I would prefer if 'finished' and 'stopping' were combined.
As it is, 'stopping' really implies finished, as you disconnect the ARI client.
'finished', on the other hand, really means 'did I set the passed state to something?' While that is a useful internal check in places, I'm not sure that should have to be conveyed outside of this class. Ideally, concrete implementations of the TestScenarios should only ever have to check if the scenario is finished (and hopefully, they don't have to do that often either)
./asterisk/trunk/tests/rest_api/applications/stasis_status/test_scenario_factory.py
<https://reviewboard.asterisk.org/r/4520/#comment25383>
So, this is where Python gets a bit different than most OO languages.
If you remove the constructor, then you are left with a single function, build_scenarios, that technically has nothing to do with the class it is defined in - there are no members of the class to manipulate. In fact, all the function does is create a list, build the scenario objects, and return the list.
Unlike Java (or some other OO languages), Python is just fine and happy with having first class functions. Thus, instead of needing an object to encapsulate the factory method, you can just have a factory method:
def build_scenarios(ami, host, port, credentials):
...
return scenarios
Then, in runtest, you simply pass the list constructed from that function as opposed to passing an instance of TestScenarioFactory. From a dependency injection perspective, the test object just needs the list of scenarios to execute, not necessarily a different factory object.
Now, if your factory had _state_ that it needed to maintain, having a factory object would make perfect sense here - but in this case, your factory method is simple enough that it doesn't warrant the class.
./asterisk/trunk/tests/rest_api/applications/stasis_status/test_scenario_factory.py
<https://reviewboard.asterisk.org/r/4520/#comment25372>
If your constructor doesn't need to do anything, you don't need to define it. You can just let the default Python constructor do its thing.
./asterisk/trunk/tests/rest_api/applications/stasis_status/test_scenario_factory.py
<https://reviewboard.asterisk.org/r/4520/#comment25384>
Rather than injecting a name, you can actually use the object type and - from it - use the name of the class as a unique way to identify the scenario:
for scenario in [BabsTestScenario, BugsTestScenario, BusterTestScenario]:
client = AriClient(host, port, credentials, scenario.__name__)
obj = scenario(client, ami)
scnearios.append(obj)
This lets you remove the name parameter passed to the TestScenario objects.
./asterisk/trunk/tests/rest_api/applications/stasis_status/test_scenario_factory.py
<https://reviewboard.asterisk.org/r/4520/#comment25373>
While this is good defensive programming, in this case, your test can trust that Asterisk will send you events that match the data model.
Asterisk, when compiled with --enable-dev-mode (which generally has to be used with running with the testsuite), will assert if a JSON blob it sends out does not match the Swagger specification. Typically, we run Asterisk with DO_CRASH enabled, which will cause Asterisk to core dump on the asserts.
Beyond that, it may actually be better to throw an exception if the data model is not correct - Asterisk should not be sending events that don't match the Swagger spec (even if someone compiled Asterisk to not assert).
Doing this also has the benefit of reducing the size of these methods by quite a lot.
./asterisk/trunk/tests/rest_api/applications/stasis_status/test_scenario_factory.py
<https://reviewboard.asterisk.org/r/4520/#comment25376>
I'm curious why you needed to check self.stopping/self.finished here.
If you've already told the scenario to stop, then you are either:
(a) Getting additional VarSet events, which you should be able to filter out by checking the Variable
(b) Have some other logic error and have stopped too early, which feels like a problem with the test logic
- Matt Jordan
On March 22, 2015, 11:34 p.m., Ashley Sanders wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4520/
> -----------------------------------------------------------
>
> (Updated March 22, 2015, 11:34 p.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: STASIS_STATUS to give outside applications context when errors occur in Stasis that interrupt normal processing.
>
> This test exercises three scenarios to elicit updates to the STASIS_STATUS channel variable:
> 1) The 'Babs' scenario: tests a nominal path of Stasis to verify the 'ACTIVE' state is correctly applied. For this test, a call is originated under normal conditions and then the system is polled for the value of STASIS_STATUS before the channel is hung up.
> 2) 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.
> 3) 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/stasis_status/test_scenario_factory.py PRE-CREATION
> ./asterisk/trunk/tests/rest_api/applications/stasis_status/test_scenario.py PRE-CREATION
> ./asterisk/trunk/tests/rest_api/applications/stasis_status/test_case.py PRE-CREATION
> ./asterisk/trunk/tests/rest_api/applications/stasis_status/test-config.yaml PRE-CREATION
> ./asterisk/trunk/tests/rest_api/applications/stasis_status/run-test PRE-CREATION
> ./asterisk/trunk/tests/rest_api/applications/stasis_status/observable_object.py PRE-CREATION
> ./asterisk/trunk/tests/rest_api/applications/stasis_status/configs/ast1/sip.conf PRE-CREATION
> ./asterisk/trunk/tests/rest_api/applications/stasis_status/configs/ast1/http.conf PRE-CREATION
> ./asterisk/trunk/tests/rest_api/applications/stasis_status/configs/ast1/extensions.conf PRE-CREATION
> ./asterisk/trunk/tests/rest_api/applications/stasis_status/configs/ast1/ari.conf PRE-CREATION
> ./asterisk/trunk/tests/rest_api/applications/stasis_status/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/20150324/34c48266/attachment-0001.html>
More information about the asterisk-dev
mailing list