[asterisk-dev] [Code Review]: Add unit tests for jitterbuf.c

[Matt Jordan]

> On March 14, 2012, 1:31 p.m., Mark Michelson wrote:
> > The factoring of common elements is a good start, but I was thinking even grander than what you had done.
> > 
> > For instance, examine the nominal voice and nominal control tests. They both call jb_new, call test_jb_populate_config, call jb_setconf, put the same number of frames with the same timestamps into the jitterbuffer, then pull the same number of frames back out and call JB_NUMERIC_TEST with the same values, then call jb_getinfo, and then finally have a bunch of calls to JB_NUMERIC_TEST again testing the same values. The only things that differ in these tests are the output of JB_TEST_BEGIN and the type of frame put into the jitterbuffer. You've factored out the portion that places the frames into the jitterbuffer, but I was thinking that essentially the ENTIRE test could be factored out since it's just the same thing being repeated twice.
> > 
> > So the nominal voice test would consist of the initial switch (cmd) statement to get things set up, and then just a function call to something like test_nominal("jitterbuffer_nominal_voice_frames", JB_TYPE_VOICE). The nominal control test would consist of the initial switch (cmd) statement to get things set up, and then just a function call to something like test_nominal("jitterbuffer_nominal_control_frames", JB_TYPE_CONTROL);
> > 
> > That being said, I'll give a "ship it" because it's not critically important.

I thought about that as well, and was looking at combining things more in a similar vein to what you suggested.  There's a few issues with doing that with some of the tests, which is why I ended up limiting it.

1. For nominal, late, and out of order, the tests are virtually identical, including the expected results.  You could effectively have a single test for each of those cases that just takes in a frame type and runs it.
2. For the lost and overflow tests, however, the expected results are not the same.  Because the jitter buffer interpolates the voice frames, you actually get valid frames back even though you never put anything into the jitter buffer for that time slot.  That's not the case for control frames.
3. Resync is obviously different, due to the fact that the jitter buffer resyncs automatically for a control frame that passes the threshold.

For tests where the expected results are different, even reading the frames out of the jitter buffer is subtly different - the return code from jb_get could be JB_INTERP instead of JB_NOFRAME, etc.  That makes it a little bit trickier to simply refactor all of the code out.

You could do something with callbacks for the portions of the tests that are different, or some other mechanism to defer the dissimilar logic to the tests calling the shared code - but that's a lot of complexity for very little pay off.

The other issue you can run into when you try to combine test code like this is when some small part of the implementation changes its behavior that invalidates previously combined sections of code.  Say for example later in the future, someone decides that when a control frame arrives out of order, the frame should be dropped.  That behavioral change would necessitate a change in the expected results of the out of order test, which would require the shared test code to be split.  That can actually make it more difficult to maintain and test then if the two tests were separated in the first place.

Sometimes the maintenance burden of having shared test code is worthwhile (as having lots of boilerplate code in tests has its own maintenance burden) - but in this case, I went with the less grand solution :-)


- Matt


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


On March 14, 2012, 9:22 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1815/
> -----------------------------------------------------------
> 
> (Updated March 14, 2012, 9:22 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch adds unit tests to cover major functions in jitterbuf.c.  This include jb_put, jb_get, jb_next, as well as general information and statistic gathering.  The scenarios include:
> 
> 1) Nominal - adding voice/control frames and making sure that all added frames are removed and in the correct time slots
> 2) Out of Order - some voice/control frames arrive out of order with respect to the previous frame.  The tests check that when reading from the jitter buffer, the frames are reordered
> 3) Lost - some voice/control frames never arrive.  In the case of voice, the jitter buffer should interpolate the frame; in the case of control, the jitter buffer should report that no frame exists at that time slot
> 4) Late - the arrival time of some voice/control frames does not occur at the same rate as surrounding frames
> 5) Resync - test that voice/control frames that break the resync threshold trigger a resync of the jitter buffer
> 6) Overflow - test that adding voice/control frames that exceed the maximum time length of the buffer are dropped
> 
> This change was prompted by https://reviewboard.asterisk.org/r/1814 - not all tests will pass without that patch.  
> 
> 
> This addresses bug ASTERISK-18964.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18964
> 
> 
> Diffs
> -----
> 
>   /trunk/tests/test_jitterbuf.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1815/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matt
> 
>

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