[asterisk-dev] [Code Review]: Add tests for SIP_CAUSE and its replacement

Tue Mar 27 11:23:30 CDT 2012

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast2/sip.conf, lines 11-27
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26860#file26860line11>
> >
> >     I think the call completion settings here are probably held over from another test

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/run-test, line 48
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26867#file26867line48>
> >
> >     To maintain the differentiation between the two tests, you may want to rename this to sc_callback

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast2/sip.conf, lines 12-28
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26866#file26866line12>
> >
> >     Call Completion settings here as well

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/run-test, lines 38-44
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26867#file26867line38>
> >
> >     If you want, you can also chain the .addErrback onto the originate calls here as well

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/run-test, lines 29-34
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26867#file26867line29>
> >
> >     Use the TestCase originate failure handler

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/tests.yaml, line 42
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26869#file26869line42>
> >
> >     Naming convention would suggest that this should be 'hangup_cause'

It is one word (HANGUPCAUSE) in dialplan usage, so changing this would probably be confusing to those using the tag.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/run-test, lines 95-97
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26867#file26867line95>
> >
> >     Start and stop here as well

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/run-test, lines 22-23
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26867#file26867line22>
> >
> >     Initialize these in the constructor (__init__).  Otherwise, they're scoped at the class level, which - while it won't cause any problems in your test - probably aren't the semantics you intended

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/run-test, line 19
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26867#file26867line19>
> >
> >     Caps lock

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast2/extensions.conf, lines 16-17
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26865#file26865line16>
> >
> >     Answer(), Echo()

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast2/extensions.conf, lines 2-7
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26865#file26865line2>
> >
> >     New line between extensions here as well

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast1/extensions.conf, lines 18-19
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26863#file26863line18>
> >
> >     Answer(), Dial() here

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast1/extensions.conf, lines 8-10
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26863#file26863line8>
> >
> >     NoOp(), Answer(), Echo()

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/test-config.yaml, line 3
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26862#file26862line3>
> >
> >     Since the name of these frames is very likely to change, you may want to rephrase the description.

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/run-test, lines 95-97
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26861#file26861line95>
> >
> >     You no longer need the start/stop asterisk pair

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/run-test, lines 39-44
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26861#file26861line39>
> >
> >     If you feel like it, you technically don't need to actually hang on to the deferred object in a local variable here.  You can, if you want, just do the following:
> >     
> >     self.ami[0].originate("local...", "dpwait", "1234", 1).addErrback(self.handleOriginateFailure)
> >     
> >

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/run-test, lines 29-34
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26861#file26861line29>
> >
> >     You can just use the originate failure handler in the TestCase class

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/run-test, lines 22-23
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26861#file26861line22>
> >
> >     Initialize these in the constructor (__init__).  Otherwise, they're scoped at the class level, which - while it won't cause any problems in your test - probably aren't the semantics you intended

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/run-test, line 19
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26861#file26861line19>
> >
> >     Same finding here regarding all caps

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast2/extensions.conf, lines 16-17
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26859#file26859line16>
> >
> >     Answer() and Echo() here

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast2/extensions.conf, lines 2-7
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26859#file26859line2>
> >
> >     For readability, add a line between each of these contexts

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast1/extensions.conf, lines 18-19
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26857#file26857line18>
> >
> >     Change to Answer() and Dial(...) here

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast1/extensions.conf, lines 8-9
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26857#file26857line8>
> >
> >     These should be:
> >     NoOp()
> >     Answer()

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast1/extensions.conf, line 4
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26857#file26857line4>
> >
> >     Remove

Done.

> On March 27, 2012, 8:45 a.m., Matt Jordan wrote:
> > asterisk/trunk/lib/python/asterisk/SimpleTestCase.py, line 48
> > <https://reviewboard.asterisk.org/r/1832/diff/1/?file=26856#file26856line48>
> >
> >     I should have caught this when this initially went in, but an all-caps object isn't really compliant with standard python nomenclature.  Granted, we've got other modules, classes, and methods that don't really follow standard nomenclature either, but we should start trying to follow it better.

Committed...

- opticron

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

On March 27, 2012, 11:23 a.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1832/
> -----------------------------------------------------------
> 
> (Updated March 27, 2012, 11:23 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Add tests for SIP_CAUSE and its replacement (currently named HANGUPCAUSE, may be something else later).  Both tests check the non-hash HANGUPCAUSE variable to ensure that no namespace collision occurs and include tests for single channel dials, branched dials, and local channel indirected branched dials.
> 
> 
> This addresses bug SWP-4223.
>     https://issues.asterisk.org/jira/browse/SWP-4223
> 
> 
> Diffs
> -----
> 
>   asterisk/trunk/tests/channels/SIP/sip_cause/test-config.yaml PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/tests.yaml 3162 
>   asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast1/extensions.conf PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast1/sip.conf PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast2/extensions.conf PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/sip_cause/configs/ast2/sip.conf PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/sip_cause/run-test PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast1/extensions.conf PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast1/sip.conf PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast2/extensions.conf PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/hangupcause/configs/ast2/sip.conf PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/hangupcause/run-test PRE-CREATION 
>   asterisk/trunk/tests/channels/SIP/hangupcause/test-config.yaml PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1832/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> opticron
> 
>

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