[asterisk-dev] [Code Review]: Add test for 'e' option on ForkCDR

Thu Apr 12 16:26:30 CDT 2012

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test, line 3
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27038#file27038line3>
> >
> >     It isn't 2010 any longer :-)

In the year 2012 2012, from a time that I'd rather not delve.
Fixed.

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/test-config.yaml, line 18
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27039#file27039line18>
> >
> >     Do we really need a ForkCDR tag?
> >     
> >     We only need tags for things that have more then one test.  If an application or Asterisk use case only has a single test that will exist for it, the --test/-t option can be used to execute that test.
> >     
> >     If you think we'll have multiple ForkCDR tests, then keep the tag.

I'll toss it for now.  There are a couple tests involving the ForkCDR application, but they are all CDR tests anyway.

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/test-config.yaml, line 2
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27039#file27039line2>
> >
> >     The summary doesn't quite make sense: "end the original end ..."?

Sorry, for some reason it seems writing summaries makes what I think and what I type cease to match up.

Changed to:
Test ForkCDR option for ending the original CDR log before starting new one

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test, lines 71-73
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27038#file27038line71>
> >
> >     You should no longer need the start_asterisk or stop_asterisk calls

Oh right. Got it.

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test, lines 54-55
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27038#file27038line54>
> >
> >     You should check that the length of cdr1 is 2 before you begin indexing into it - if only one CDR record was created, this would throw an exception.

if that were the case, we would already have returned on account of CDRTestCase.match_cdrs(self) above.

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test, lines 60-61
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27038#file27038line60>
> >
> >     Same here - make sure that end and start time are not None.

For these two, I've added a check to make sure none of these is none in a for loop that iterates through the array.

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test, line 24
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27038#file27038line24>
> >
> >     Typically, we haven't been naming class names with the '_' character.  That tends to go against the current naming conventions (such as they are), as well as typical Python naming conventions for classes.
> >     
> >     See http://www.python.org/dev/peps/pep-0008/#naming-conventions for more information on what to avoid.
> >     
> >     (That isn't to say we aren't violating the above conventions elsewhere in the code, but that's no reason to continue doing so)

Switched to CamelCase.

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test, lines 35-36
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27038#file27038line35>
> >
> >     The '))' does not need to be on its own line.  Add spaces around your '=' signs.

Parenthesis kicked up.

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test, lines 44-45
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27038#file27038line44>
> >
> >     The '))' does not need to be on its own line.  Add spaces around your '=' signs.

Parenthesis kicked around.

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test, line 49
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27038#file27038line49>
> >
> >     if (not self.passed)

if not self.passed:

> On April 12, 2012, 8:21 a.m., Matt Jordan wrote:
> > /asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test, line 52
> > <https://reviewboard.asterisk.org/r/1853/diff/1/?file=27038#file27038line52>
> >
> >     Space around the '='

spaciated.

- jrose

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

On April 6, 2012, 10:42 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1853/
> -----------------------------------------------------------
> 
> (Updated April 6, 2012, 10:42 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson, Paul Belanger, rmudgett, and Matt Jordan.
> 
> 
> Summary
> -------
> 
> This is a fairly simple CDRTestCase test for checking that ForkCDR behaves as expected with the e option.  It was made in response to the above bug.
> 
> Basically it just checks the expectations and then makes sure that the CDR logs both have some duration greater than 1 second (significant wiggle room).  Then it checks to
> see if the end time of the first came within one second of the start time of the last (the threshold is to accomodate for possible timing issues)
> 
> 
> This addresses bug ASTERISK-19164.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19164
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/cdr/cdr_fork_end_time/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/cdr/cdr_fork_end_time/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test PRE-CREATION 
>   /asterisk/trunk/tests/cdr/cdr_fork_end_time/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/cdr/tests.yaml 3170 
> 
> Diff: https://reviewboard.asterisk.org/r/1853/diff
> 
> 
> Testing
> -------
> 
> With Asterisk not having the patch from ASTERISK-19164, the test fails due to the duration of the second CDR being 0.
> With Asterisk having the patch, the test passes.  Also all the other CDR tests continue to pass as expected.
> 
> 
> Thanks,
> 
> jrose
> 
>

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