[asterisk-dev] [Code Review]: Add test for correct HANGUPCAUSE after SIP 404

[wdoekes]

> On May 31, 2012, 9:17 a.m., opticron wrote:
> > /asterisk/trunk/tests/channels/SIP/sip2cause/run-test, line 36
> > <https://reviewboard.asterisk.org/r/1951/diff/2/?file=28327#file28327line36>
> >
> >     This line shouldn't be necessary since you're creating the AMI factory in run() below.

I don't think the AMI factory in run() is relevant.

  https://reviewboard.asterisk.org/r/1606/ <-- Paul says I should add it
  https://reviewboard.asterisk.org/r/1606/diff/2-3/

And calling it is a no-op (until someone implements useful functionality there):

  class TestCase:
      ...
      def ami_connect(self, ami):
          """
          Hook method used after create_ami_factory() successfully logs into
          the Asterisk AMI.
          """
          pass

Out of 177 tests, I find *at* *least* 42 that do it:

  $ find . -name run-test | wc -l
  177
  $ find . -name run-test | xargs grep def\ ami_connect -A1 | grep -F .ami_connect | wc -l
  42

I don't mind removing the no-op, but I'd like some consensus first.


> On May 31, 2012, 9:17 a.m., opticron wrote:
> > /asterisk/trunk/tests/channels/SIP/sip2cause/run-test, line 61
> > <https://reviewboard.asterisk.org/r/1951/diff/2/?file=28327#file28327line61>
> >
> >     This isn't necessary since you haven't registered this function for any callbacks other than Hangup.

Yes, but I like assertions. So I'm leaving it in there.


> On May 31, 2012, 9:17 a.m., opticron wrote:
> > /asterisk/trunk/tests/channels/SIP/sip2cause/run-test, lines 43-48
> > <https://reviewboard.asterisk.org/r/1951/diff/2/?file=28327#file28327line43>
> >
> >     You should be able to leave the error callback out entirely if you don't want to handle that case.

No I cannot, because that will mean that starpy.error.AMICommandFailure() is unhandled and that one goes and kills the deferred thread.


> On May 31, 2012, 9:17 a.m., opticron wrote:
> > /asterisk/trunk/tests/channels/SIP/sip2cause/run-test, lines 79-80
> > <https://reviewboard.asterisk.org/r/1951/diff/2/?file=28327#file28327line79>
> >
> >     Just a nitpick, there is no need to complicate this with a list if you aren't going to use the information later.  A simple count will suffice.

Yea.. I was thinking I'd use it for logging, but I won't.

Replaced.


> On May 31, 2012, 9:17 a.m., opticron wrote:
> > /asterisk/trunk/tests/channels/SIP/sip2cause/run-test, line 87
> > <https://reviewboard.asterisk.org/r/1951/diff/2/?file=28327#file28327line87>
> >
> >     This line is no longer required.

Removed.


> On May 31, 2012, 9:17 a.m., opticron wrote:
> > /asterisk/trunk/tests/channels/SIP/sip2cause/run-test, line 89
> > <https://reviewboard.asterisk.org/r/1951/diff/2/?file=28327#file28327line89>
> >
> >     Ditto.

Removed.


- wdoekes


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


On May 29, 2012, 3:57 a.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1951/
> -----------------------------------------------------------
> 
> (Updated May 29, 2012, 3:57 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> See ASTERISK-19914 and preceding discussion in http://lists.digium.com/pipermail/asterisk-dev/2012-May/055309.html
> 
> Apparently r351130 broke correct HANGUPCAUSE setting in certain cases. This only tests the 404 case. Feel free to expand it later if needed.
> 
> 
> P.S. There are now 3 test cases that concern hangupcauses. That might cause confusion:
>   hangupcause => tests ${HASH*(HANGUPCAUSE,*)}
>   sip_cause => tests ${HASH*(SIP_CAUSE,*)}
>   sip2cause => tests _getting_ ${HANGUPCAUSE} (and implicit propagation)
> 
> 
> This addresses bug ASTERISK-19914.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19914
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/channels/SIP/sip2cause/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip2cause/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip2cause/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip2cause/sipp/invite_recv_do_404.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip2cause/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/tests.yaml 3241 
> 
> Diff: https://reviewboard.asterisk.org/r/1951/diff
> 
> 
> Testing
> -------
> 
> Before chan_sip.diff from ASTERISK-19914 on 1.8.x.
> 
> <?xml version="1.0" encoding="utf-8"?>
> <testsuite errors="0" failures="1" name="AsteriskTestSuite" tests="1" time="7.43">
>   <testcase name="tests/channels/SIP/sip2cause" time="7.43">
>     <failure>
>       Running ['tests/channels/SIP/sip2cause/run-test'] ...
> [May 29 08:31:35] ERROR[14215]: __main__:69 on_hangup: Unexpected HANGUPCAUSE 34 (not 1) for channel SIP/alice-00000000
> [May 29 08:31:40] ERROR[14215]: __main__:69 on_hangup: Unexpected HANGUPCAUSE 0 (not 1) for channel Local/1111 at dial-alice-1c03;1
> [May 29 08:31:40] ERROR[14215]: __main__:69 on_hangup: Unexpected HANGUPCAUSE 34 (not 1) for channel Local/1111 at dial-alice-1c03;2
> 
>     </failure>
>   </testcase>
> </testsuite>
> 
> 
> After chan_sip.diff on 1.8.x (and on 1.6.2.x):
> 
> <?xml version="1.0" encoding="utf-8"?>
> <testsuite errors="0" failures="0" name="AsteriskTestSuite" tests="1" time="12.54">
>   <testcase name="tests/channels/SIP/sip2cause" time="12.54"/>
> </testsuite>
> 
> 
> Thanks,
> 
> wdoekes
> 
>

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