> 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
