----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3660/#review12339 -----------------------------------------------------------
/asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22516> There's no reason to define an Originate as a generic message. The starpy AMIProtocol already defines an explicit method for origination. /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22518> Since you've decided to implement a new test object for your test, you should make sure it passes pylint. That means all of your class declarations and function definitions should have Pydoc comments /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22517> If you're going to explicitly lower the default reactor timeout, then you should reset the reactor timeout periodically. You don't know how slow a build agent will run. /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22519> Pydoc comments /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22525> (1) You don't need a global for this. You can just use the originate method on the AMIProtocol. (2) Add the default error handler for originates, defined in TestCase. That way, if for whatever reason a channel doesn't get answered, your test will fail appropriately. /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22526> This should not set the test to pass here. You should only pass the test if the channels that are originated are successfully created (that is, answered). You can use a deferred list to gather up the deferreds returned by the AMIProtocol originate. When all deferreds have fired, the deferred list will fire, which will call the callback you've defined for the deferred list. By default, the first parameter passed to a callback of a deferred list is a list of tuples (success, result) where success is a boolean indicating whether or not the callback succeeded. You can then set the test to passed if all of the originates succeeded. Something like: def _pass_test(results, test_object): passed = all([result[0] for result in results if result[0]) test_object.set_passed(passed) deferds = [] for channel in self.originates: deferred = self.ami.originate(...) deferred.addErrback(self.handle_originate_failure) deferds.append(deferred) deferred_list = defer.DeferredList(deferds) deferred_list.addCallback(_pass_test, self) /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22520> Pydoc /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22521> (1) This method is named poorly. What are we expecting? (2) Since this is a response callback function for an executed action, and is called no where else, this method could be scoped under the ami_connect method. It does not need to be scoped at the class level. (3) And Pydoc this as well. /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22532> Why are these type checks necessary? When does AMIProtocol send you something other than a dictionary as the response? /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22522> (1) This method is named poorly. What are we unexpecting? (2) Since this is a response callback function for an executed action, and is called no where else, this method could be scoped under the ami_connect method. It does not need to be scoped at the class level. (3) And Pydoc this as well. /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22523> Py to the Doc /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22524> And one more time: PyDoc /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22531> "Command" should be lower case. /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py <https://reviewboard.asterisk.org/r/3660/#comment22529> Change this to "expected-response" /asterisk/trunk/tests/pbx/manager_extensions/test-config.yaml <https://reviewboard.asterisk.org/r/3660/#comment22530> Keys and parameters typically follow snake_case (or snake-case, I suppose), unless the keys are going to be directly fed into an AMI action. These should be: expected-response (or expected_response) command /asterisk/trunk/tests/pbx/manager_extensions/test-config.yaml <https://reviewboard.asterisk.org/r/3660/#comment22528> I'd name this YAML key "expected_response". - Matt Jordan On June 20, 2014, 1:42 p.m., Jonathan Rose wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3660/ > ----------------------------------------------------------- > > (Updated June 20, 2014, 1:42 p.m.) > > > Review request for Asterisk Developers, kmoore and Matt Jordan. > > > Repository: testsuite > > > Description > ------- > > 11 part test: > 1 - Remove an extension with a priority > If the manager command fails, a failure token will be set > If the original extension runs, a user event will be generated that > makes the test fail > > 2 - Remove an entire existing extension > If the manager command fails, a failure token will be set > If the original extension runs, a user event will be generated that > makes the test fail > > 3 - Add an extension > If the manager command fails, a failure token will be set > If the extension fails to run, an expected user event will not be > generated and the test will fail > > 4 - Add an extension with a CID match that doesn't match the originated > channel > If the manager command fails, a failure token will be set > If the extension runs, a user event will be generated that makes the > test fail > > 5 - Add an extension with a CID match that should match the originated > channel > If the manager command fails, a failure token will be set > If the extension fails to run, an expected user event will not be > generated and the test will fail > > 6 - Add an extension that replaces an existing priority > If the manager command fails, a failure token will be set > If the original extension runs, a user event will be generated that > makes the test fail > If the replacement extension doesn't run, an expected user event will > not be generated and the test will fail > > 7 - Attempt to add an extension that would replace an existing priority, but > don't allow replacement > If the manager command doesn't fail, a failure token will be set > If the original extension doesn't run, an expected user event will not > be generated and the test will fail > If the replacement extension runs, a user event will be generated that > makes the test fail > > 8 - Remove an extension at a specific priority with caller ID matching > If the manager command fails, a failure token will be set > If the original extension runs, a user event will be generated that > makes the test fail > The originate is checked by means of a user event on a separate priority > taht is not removed > > 9 - Attempt DialplanAddExtension without priority set > If the manager command doesn't fail, a failure token will be set > > 10 - Attempt DialplanAddExtension without application set > If the manager command doesn't fail, a failure token will be set > > 11 - Attempt DialplanRemoveExtension on an extension that does not exist > If the manager command doesn't fail, a failure token will be set > > > Diffs > ----- > > /asterisk/trunk/tests/pbx/tests.yaml 5144 > /asterisk/trunk/tests/pbx/manager_extensions/test-config.yaml PRE-CREATION > /asterisk/trunk/tests/pbx/manager_extensions/configs/ast1/extensions.conf > PRE-CREATION > /asterisk/trunk/tests/pbx/manager_extensions/ami_extension_control.py > PRE-CREATION > > Diff: https://reviewboard.asterisk.org/r/3660/diff/ > > > Testing > ------- > > Removed execution of many of the test AMI commands to guarantee that if above > failures occurred that the test would fail > > > Thanks, > > Jonathan Rose > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
