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

[Matt Jordan]

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



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10947>

    It isn't 2010 any longer :-)



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10946>

    You may need to check this, but you shouldn't need the Asterisk or TestCase imports, since your test doesn't directly interact with an Asterisk instance and your test derives from a class that itself derives from TestCase.



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10938>

    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)



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10939>

    The '))' does not need to be on its own line.  Add spaces around your '=' signs.



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10940>

    The '))' does not need to be on its own line.  Add spaces around your '=' signs.



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10942>

    if (not self.passed)



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10941>

    Space around the '='



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10945>

    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.



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10943>

    If, for whatever reason, duration was not written to the CSV CDR, this would throw an exception.  Since the initial value of duration is None, you should first check if the duration values are None, and fail the test if they are.



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10944>

    Same here - make sure that end and start time are not None.



/asterisk/trunk/tests/cdr/cdr_fork_end_time/run-test
<https://reviewboard.asterisk.org/r/1853/#comment10937>

    You should no longer need the start_asterisk or stop_asterisk calls



/asterisk/trunk/tests/cdr/cdr_fork_end_time/test-config.yaml
<https://reviewboard.asterisk.org/r/1853/#comment10948>

    The summary doesn't quite make sense: "end the original end ..."?



/asterisk/trunk/tests/cdr/cdr_fork_end_time/test-config.yaml
<https://reviewboard.asterisk.org/r/1853/#comment10949>

    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.


- Matt


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/9faa3869/attachment-0001.htm>