[asterisk-dev] [Code Review]: Testsuite test for hangup handlers.

[Matt Jordan]

> On July 25, 2012, 1:57 p.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/hangup/handlers/run-test, lines 52-66
> > <https://reviewboard.asterisk.org/r/2062/diff/1/?file=30618#file30618line52>
> >
> >     If nothing else, each of these checks could be refactored into smaller subroutines that verify tests of each particular type.
> 
> rmudgett wrote:
>     This code is not verifying the test results.  It is setting up the expected test parameters.

Okay, then refactor it into sub-routines that set up the expected test parameters.  Giant methods of indented python code makes people go blind.

Plus, if you go with the previous suggestion of using data structures to set this information up, this is unnecessary.


> On July 25, 2012, 1:57 p.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/hangup/handlers/run-test, lines 80-81
> > <https://reviewboard.asterisk.org/r/2062/diff/1/?file=30618#file30618line80>
> >
> >     There's no reason not to have a single routine that resets your test state.  The "cost" of setting variables to 0 when they may already be 0 is offset by the fact that you are, in fact, running in Python.
> 
> rmudgett wrote:
>     You cannot have a routine to do this.  It would terminate the entire test and not switch to the next sub-test sequence.  This code causes the code to get to the next elif in the elif ladder the next time around.

You're correct.

This highlights, however, a problem with this test: having this many tracking member variables is confusing in the extreme.  I confused self.no_call_w_h_tests with your counter variables.

I would prefer a solution that does not rely on having numerous flags and counter to track which test you happen to be on.


> On July 25, 2012, 1:57 p.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/hangup/handlers/run-test, lines 310-314
> > <https://reviewboard.asterisk.org/r/2062/diff/1/?file=30618#file30618line310>
> >
> >     Assuming you don't want to auto fail the test when a failing condition happens, but you also don't want the test to accidentally pass later on, you can now use self.passed() to set the pass/fail status of the test.
> >     
> >     Setting self.passed(False) will set the test pass status to False, and will prevent a later self.passed(True) from overwriting the failed state.
> 
> rmudgett wrote:
>     The sub-test fails when one of these events happens.  I don't want to terminate the entire test.  The entire test pass/fails if all of the sub-tests pass.

I'm not suggesting you terminate the entire test.  What I'm saying is you can eliminate your counter / saw_failed variables.  Instead of maintaining a count of all of the 'things' you saw in an event, simply set self.passed(False).  That will fail the test, but will not stop the reactor.

Then you only need to maintain a count of the tests you've completed successfully.  If you complete all tests successfully - by receiving back the correct number of UserEvents - you set the test passed status to True - self.passed(True).  This cannot overwrite a previous call of self.passed(False).

You reduce the number of counters you need to, at most, 1.


- Matt


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


On July 25, 2012, 12:19 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2062/
> -----------------------------------------------------------
> 
> (Updated July 25, 2012, 12:19 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Tests to test hangup handlers:
> 1) Tests relative hangup handler location resolution.
> 2) Tests hangup_handler_pop and hangup_handler_wipe
> 3) Tests hangup handler stack execution order
> 4) Tests hangup handler execution on both sides of a single call with and without an h-exten.
> 5) Tests hangup handler execution on a forked call with and without an h-exten.
> 6) First half of CDR endbeforehexten test.  Tests the default no setting.
> 
> 
> This addresses bug SWP-4207.
>     https://issues.asterisk.org/jira/browse/SWP-4207
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/hangup/handlers/configs/ast1/cdr.conf PRE-CREATION 
>   /asterisk/trunk/tests/hangup/handlers/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/hangup/handlers/run-test PRE-CREATION 
>   /asterisk/trunk/tests/hangup/handlers/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/hangup/tests.yaml PRE-CREATION 
>   /asterisk/trunk/tests/tests.yaml 3361 
> 
> Diff: https://reviewboard.asterisk.org/r/2062/diff
> 
> 
> Testing
> -------
> 
> All tests pass.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120725/c3c98897/attachment-0001.htm>