[asterisk-dev] [Code Review] Various unit test API updates

[Russell Bryant]

> On 2010-02-09 11:25:20, David Vossel wrote:
> > /trunk/include/asterisk/test.h, line 128
> > <https://reviewboard.asterisk.org/r/493/diff/1/?file=8045#file8045line128>
> >
> >     Should we const the ast_test in these macros? I guess since we don't define the ast_test in the header the test functions don't actually know what an ast_test is, but it would be nice if they couldn't modify it at all.

The only usage of the argument requires a non const reference to the ast_test.  It is passed to the status update function, which will modify the ast_str status log.


> On 2010-02-09 11:25:20, David Vossel wrote:
> > /trunk/main/test.c, line 224
> > <https://reviewboard.asterisk.org/r/493/diff/1/?file=8047#file8047line224>
> >
> >     This is going to contain a log of updates as well as errors.  Will that make this field more verbose than necessary?

It will certainly become more verbose, but I think it's useful to have a full log of the test.  Perhaps we should only include it if the test failed, like is done in the XML case?


> On 2010-02-09 11:25:20, David Vossel wrote:
> > /trunk/tests/test_utils.c, line 75
> > <https://reviewboard.asterisk.org/r/493/diff/1/?file=8055#file8055line75>
> >
> >     All these double \n or log strings beginning with a \n need to change to just being the log sting ending with a single \n.  This was my code, and it is dumb. We probably don't even need all these status updates on success to begin with.  If you don't change this I will afterwards.

I think I removed the double messages already.  As for extra \ns, I'll take a look for them to include a fix in my commit.  I don't mind.


- Russell


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


On 2010-02-08 19:23:00, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/493/
> -----------------------------------------------------------
> 
> (Updated 2010-02-08 19:23:00)
> 
> 
> Review request for Asterisk Developers and David Vossel.
> 
> 
> Summary
> -------
> 
> This is a bit of a mix of changes to the test API that were a part of what I worked on during part of a long flight.  Here is a breakdown of changes included here:
> 
> 1) It occurred to me that the difference in usage between the error ast_str and the ast_test_update_status() usage has turned out to be a bit ambiguous in practice.  In a lot of cases, the same message was being sent to both.  In other cases, it was only sent to one or the other.  My opinion now is that in every case, I think it makes sense to do both; we should output it to the CLI as well as save it off for logging purposes.
> 
> This change results in most of the changes in this diff, since it required changes to all existing unit tests.  It also allowed for some simplifications of unit test API implementation code.
> 
> 2) Update ast_test_status_update() to include the file, function, and line number for the code providing the update.
> 
> 3) There are some formatting tweaks here and there.  Hopefully they aren't too distracting for code review purposes.  Reviewboard's diff viewer seems to do a pretty good job of pointing out when something is a whitespace change.
> 
> 4) I moved the md5_test and sha1_test into the test_utils module.  It seemed like a better approach since these tests are so tiny.
> 
> 5) I changed the number of nodes used in heap_test_2 from 1 million to 100 thousand.  The only reason for this was to reduce the time it took for this test to run.
> 
> 6) Remove an unused function prototype that was at the bottom of utils.h.
> 
> 7) Simplify test_insert() using the LIST_INSERT_SORTALPHA() macro.  The one minor difference in behavior is that it no longer checks for a test registered with the same name.
> 
> 8) Expand the code in test_alloc() to provide specific error messages for each failure case, to clearly inform developers if they forget to set the name, summary, description, etc.
> 
> 9) Tweak the output of the "test show registered" CLI command.  I swapped the name and category to have the category first.  It seemed more natural since that is the sort key.
> 
> 10) Don't output the status ast_str in the "test show results" CLI command.  This is going to tend to be pretty verbose, so just leave that for the detailed test logs (test generate results).
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/sip/config_parser.c 245626 
>   /trunk/channels/sip/reqresp_parser.c 245626 
>   /trunk/funcs/func_math.c 245626 
>   /trunk/include/asterisk/test.h 245626 
>   /trunk/include/asterisk/utils.h 245626 
>   /trunk/main/test.c 245626 
>   /trunk/tests/test_ast_format_str_reduce.c 245626 
>   /trunk/tests/test_heap.c 245626 
>   /trunk/tests/test_md5.c 245626 
>   /trunk/tests/test_sched.c 245626 
>   /trunk/tests/test_sha1.c 245626 
>   /trunk/tests/test_skel.c 245626 
>   /trunk/tests/test_substitution.c 245626 
>   /trunk/tests/test_utils.c 245626 
> 
> Diff: https://reviewboard.asterisk.org/r/493/diff
> 
> 
> Testing
> -------
> 
> I have run the tests in the test suite to make sure they all still pass.  I have also run through the CLI commands to verify that I get the expected output.
> 
> 
> Thanks,
> 
> Russell
> 
>