[asterisk-dev] [Code Review]: SDP Media Attribute Interface + H.263/H.264 Format Attribute Modules

[Matthew Jordan]

----- Original Message -----
> From: "Paul Belanger" <paul.belanger at polybeacon.com>
> To: asterisk-dev at lists.digium.com
> Sent: Tuesday, July 3, 2012 9:22:23 AM
> Subject: Re: [asterisk-dev] [Code Review]: SDP Media Attribute Interface + H.263/H.264 Format Attribute Modules
> 
> On 12-07-02 01:26 PM, Joshua Colp wrote:
> >
> >
> >> On June 27, 2012, 11:19 a.m., Mark Michelson wrote:
> >>> I'm giving a "ship it" due to the testing you said you've done.
> >>> I'd feel better if there were unit tests added to ensure that
> >>> SDP is parsed and generated properly.
> >>
> >> Paul Belanger wrote:
> >>      Agreed, I think also adding unit tests before 'ship it' would
> >>      be acceptable here.
> >
> > Unit tests are a separate issue, but I wanted to get eyes on this
> > before going down that road.
> >
> Why separate?  Trying to understand why we as a project don't focus
> more
> on unit tests first before writing code and merging it.

I believe that is up to the developer and the review process.  If reviewers
feel that the code change is sufficiently complex and has a high degree of risk
associated with it, then it is a valid finding against that code change to
require unit or functional tests.  Its certainly fine as well to post the tests
on a separate review to keep the code reviews manageable.

Answering the question of TDD - which I think you're alluding to - is much
more complex.  TDD provides a lot of benefits, but also has significant costs.
While I personally am a firm advocate of TDD, particularly for new projects 
and - in some cases, new code - switching your software development processes to
TDD requires significant thought and careful consideration. It also incurs a
significant burden on open source contributors.

I will say that the vast majority of new features in Asterisk have some level
of tests, and the new features that do not have tests either do not because:
a) The necessary tools to test them don't exist yet (see: WebSockets)
b) The tools to test them are still under evaluation and we're working on it
 (see: res_jingle2)
c) We're working on the tests concurrently, but we're trying to review things
in order of priority.

I fully expect the new features going into Asterisk 11 to have tests covering
their core functionality by the time the first release is made.

> Is it a time thing or just too much work getting a box setup for unit
> tests?  Also, do people see value in having our code review tool run
> 'tests' before said patch is merged into svn?
> 

I don't see this as a function of the code review tool, so much as the
continuous integration process.  Having used such systems in the past, there
is a lot of benefit in having the continuous integration process reject
check-ins if some test fails; however, that would require significant tool
integration and process changes that would require some thought.

--
Matthew Jordan
Digium, Inc. | Software Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: http://digium.com & http://asterisk.org