[asterisk-dev] [Code Review] 3734: media formats: Add a 'none' format to prevent NULL format derefences; update unit tests

[Matt Jordan]

> On July 11, 2014, 1:43 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed-trunk/tests/test_core_format.c, lines 61-66
> > <https://reviewboard.asterisk.org/r/3734/diff/4/?file=62711#file62711line61>
> >
> >     Do we really want doxygen tags on unit tests?

Yes!

(1) It's generally harmless.
(2) Tests can be just as complex as 'regular' code. In this particular case, I'm implementing a format attribute module inside the test; that probably warrants some documentation
(3) When tests do fail, it can be incredibly challenging to understand why they failed without adequate documentation regarding what they do

While I would never be as stringent about doxygen documentation for unit tests - they are unit tests, and are not an API to be consumed by other developers - I find that going back to a unit test written a year ago that has reasonable documentation is much nicer than not. Exhibit A: test_taskprocessor vs test_poll.


> On July 11, 2014, 1:43 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed-trunk/tests/test_core_format.c, line 125
> > <https://reviewboard.asterisk.org/r/3734/diff/4/?file=62711#file62711line125>
> >
> >     Not a big deal since this is a unit test, but why not replace with 'if (pvt1 == pvt2)'?  If the pointer is the same the formats are equal.

Fixed.


> On July 11, 2014, 1:43 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed-trunk/tests/test_format_cap.c, line 359
> > <https://reviewboard.asterisk.org/r/3734/diff/4/?file=62713#file62713line359>
> >
> >     Can we make this mention that the codec id is already in the caps?  Since this string should not print it is mostly for documentation.  This way it's clearer from here why the cap_count should still be 1.

"Adding of duplicate format to capabilities structure failed"
"Adding of duplicate named format to capabilities structure failed"


> On July 11, 2014, 1:43 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed-trunk/tests/test_format_cap.c, line 478
> > <https://reviewboard.asterisk.org/r/3734/diff/4/?file=62713#file62713line478>
> >
> >     Should we also test that the counts from original dst_caps + src_caps == count?

Added:
 	ast_test_validate(test, ast_format_cap_count(dst_caps) == ast_format_cap_count(src_caps));


- Matt


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


On July 10, 2014, 11:11 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3734/
> -----------------------------------------------------------
> 
> (Updated July 10, 2014, 11:11 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch does two things:
> 
> * It updates a few of the unit tests for some of the API changes. In particular, it focuses on adding some tests for formats with attributes and their expected behaviour. A few other non-format related unit tests were updated as well to handle off nominals detected during testing.
> 
> * It adds an 'ast_format_none' format. This format is a dummy format that can be used instead of a NULL pointer to prevent having to put NULL dereference checks into every place in the codebase. Channels are no assigned this format immediately upon creation, and their default capabilities are set to have it. As this format's codec has no translation (nor a representation in the RTP engine), it _shouldn't_ cause harm.
> 
> * A few NULL checks were put in anyway into key areas in a few modules. These were ones that were hit hard by the unit tests and prone to crashing if presented a NULL format.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed-trunk/tests/test_format_cap.c 418325 
>   /team/group/media_formats-reviewed-trunk/tests/test_format_cache.c 418325 
>   /team/group/media_formats-reviewed-trunk/tests/test_core_format.c 418325 
>   /team/group/media_formats-reviewed-trunk/tests/test_cel.c 418325 
>   /team/group/media_formats-reviewed-trunk/main/format_cap.c 418325 
>   /team/group/media_formats-reviewed-trunk/main/format_cache.c 418325 
>   /team/group/media_formats-reviewed-trunk/main/codec_builtin.c 418325 
>   /team/group/media_formats-reviewed-trunk/main/codec.c 418325 
>   /team/group/media_formats-reviewed-trunk/main/channel.c 418325 
>   /team/group/media_formats-reviewed-trunk/main/bridge_channel.c 418325 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format_cache.h 418325 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/codec.h 418325 
> 
> Diff: https://reviewboard.asterisk.org/r/3734/diff/
> 
> 
> Testing
> -------
> 
> Unit tests pass.
> 
> There is a FRACK on shutdown, but it doesn't appear to be caused by this patch (things didn't run long enough without this patch before)
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140711/db5c1d1e/attachment.html>