[asterisk-dev] [Code Review]: Test Queue(F)

[jrose]

> On March 5, 2012, 10:59 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/queues/queue_transfer_callee/configs/ast1/modules.conf, lines 1-16
> > <https://reviewboard.asterisk.org/r/1789/diff/2/?file=25298#file25298line1>
> >
> >     None of this should be necessary

removed modules.conf


> On March 5, 2012, 10:59 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/queues/queue_transfer_callee/run-test, line 47
> > <https://reviewboard.asterisk.org/r/1789/diff/2/?file=25300#file25300line47>
> >
> >     This should be a debug statement - not every UserEvent needs to be sent to messages.txt

changed.


> On March 5, 2012, 10:59 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/queues/queue_transfer_callee/run-test, line 53
> > <https://reviewboard.asterisk.org/r/1789/diff/2/?file=25300#file25300line53>
> >
> >     Instead of if event['userevent'] == 'Alpha', check to see if its not Alpha and return.  That would reduce the indentation needed here.

I did this to reduce nesting and I went ahead and changed how the event test number is compared to the expected test number to reduce nesting as well.


> On March 5, 2012, 10:59 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/queues/queue_transfer_callee/run-test, line 65
> > <https://reviewboard.asterisk.org/r/1789/diff/2/?file=25300#file25300line65>
> >
> >     What is expectation?
> >     
> >     In general, if you're going to print out an error message, it should give someone enough information to understand why something failed.
> >     
> >     "Event test number [3] did not match expected event test number [2]" - or something along those lines - would be much more helpful to someone debugging why a test failed.
> >     
> >     And you don't need to proceed if the test failed.  If it failed, you can stop the reactor - if your tests get out of sync, then every test after that is very likely to be off as well.

Fixed.


> On March 5, 2012, 10:59 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/queues/queue_transfer_callee/test-config.yaml, line 4
> > <https://reviewboard.asterisk.org/r/1789/diff/2/?file=25302#file25302line4>
> >
> >     witha => with a
> >     
> >     You may want to reword this, since its not clear from this what "F options" actually is.
> >     
> >     Its also not clear who the 'callee' is in this case.  You should probably elaborate on what this actually means, and how the option is triggered (for example, "when the caller hangs up, the called party is transferred to some other extension specified by the F option")

Reworked the description entirely.


- jrose


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


On March 2, 2012, 3:01 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1789/
> -----------------------------------------------------------
> 
> (Updated March 2, 2012, 3:01 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson, Paul Belanger, and Matt Jordan.
> 
> 
> Summary
> -------
> 
> Adds a test for new Queue Option F.
> 
> This is basically just like the Baseline Queue Test, only there are 4 scenarios tested and they all use QueueF with different arguments.
> 
> 
> This addresses bug ASTERISK-19283.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19283
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/queues/queue_transfer_callee/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/queues/queue_transfer_callee/configs/ast1/modules.conf PRE-CREATION 
>   /asterisk/trunk/tests/queues/queue_transfer_callee/configs/ast1/queues.conf PRE-CREATION 
>   /asterisk/trunk/tests/queues/queue_transfer_callee/run-test PRE-CREATION 
>   /asterisk/trunk/tests/queues/queue_transfer_callee/run-test.lua PRE-CREATION 
>   /asterisk/trunk/tests/queues/queue_transfer_callee/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/queues/tests.yaml 3072 
> 
> Diff: https://reviewboard.asterisk.org/r/1789/diff
> 
> 
> Testing
> -------
> 
> It's a test.  I ran it a number of times, checked log messages, etc.  Nothing too special.
> 
> 
> Thanks,
> 
> jrose
> 
>

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