<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3734/">https://reviewboard.asterisk.org/r/3734/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 11th, 2014, 2:43 a.m. EDT, <b>Corey Farrell</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/3734/diff/4/?file=62711#file62711line61" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed-trunk/tests/test_core_format.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">61</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm">/*! \brief A test piece of data to associate with \ref test_core_format_attr */</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">62</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">struct</span> <span class="n">test_core_format_pvt</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">63</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/*! Some data field */</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">64</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="kt">int</span> <span class="n">field_one</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">65</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/*! Another arbitrary data field */</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">66</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="kt">int</span> <span class="n">field_two</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Do we really want doxygen tags on unit tests?</pre>
</blockquote>
<p>On July 11th, 2014, 12:42 p.m. EDT, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">For the record I wasn't suggesting that the comments should be removed, just questioning if we want this to be part of the HTML API documentation that is generated. I think comments in unit test source files are good, but if they translate to generated HTML documentation that is clutter.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 11th, 2014, 2:43 a.m. EDT, <b>Corey Farrell</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/3734/diff/4/?file=62713#file62713line478" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed-trunk/tests/test_format_cap.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">478</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">count</span> <span class="o">=</span> <span class="n">ast_format_cap_count</span><span class="p">(</span><span class="n">dst_caps</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Should we also test that the counts from original dst_caps + src_caps == count?</pre>
</blockquote>
<p>On July 11th, 2014, 12:42 p.m. EDT, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Added:
ast_test_validate(test, ast_format_cap_count(dst_caps) == ast_format_cap_count(src_caps));
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Sorry I was unclear, what I meant:
/* line 466 */ total_count = ast_format_cap_count(dst_caps) + ast_format_cap_count(src_caps);
/* line 486 */ ast_test_validate(test, total_count == ast_format_cap_count(dst_caps));
This way total_count is the count of all audio + all video codecs. After ast_format_cap_append_from_cap we want to verify dst_cap has the combined count. I expect the test you added will fail (I haven't tried it so I could be wrong).</pre>
<br />
<p>- Corey</p>
<br />
<p>On July 11th, 2014, 12:55 p.m. EDT, Matt Jordan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated July 11, 2014, 12:55 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/team/group/media_formats-reviewed-trunk/tests/test_format_cap.c <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/tests/test_format_cache.c <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/tests/test_core_format.c <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/tests/test_cel.c <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/format_cap.c <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/format_cache.c <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/codec_builtin.c <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/codec.c <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/channel.c <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/bridge_channel.c <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/format_cache.h <span style="color: grey">(418370)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/codec.h <span style="color: grey">(418370)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3734/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>