[opensource-dev] Review Request: KDU Improvements: add unit tests for llkdu

[Merov Linden]

> On 2010-12-23 06:46:09, Vadim ProductEngine wrote:
> > indra/llkdu/tests/llimagej2ckdu_test.cpp, line 193
> > <http://codereview.secondlife.com/r/63/diff/1/?file=256#file256line193>
> >
> >     What about calling protected methods via inheritance?

IIRC the tut framework (used to create unit tests) prevents that to be done.


> On 2010-12-23 06:46:09, Vadim ProductEngine wrote:
> > indra/llkdu/tests/llimagej2ckdu_test.cpp, lines 200-203
> > <http://codereview.secondlife.com/r/63/diff/1/?file=256#file256line200>
> >
> >     I suppose unit tests should verify something more substantial than not raising exceptions for empty data.

Frustrating I know and not enough to truly test the implementation. Unit tests though are supposed to stay "pure" and test only that the public API calls are always safe even in an "empty" environment. To do more, I should be creating "Integration tests". We also use tut for that and I'm planning to do that next.


> On 2010-12-23 06:46:09, Vadim ProductEngine wrote:
> > indra/llkdu/llimagej2ckdu.h, line 58
> > <http://codereview.secondlife.com/r/63/diff/1/?file=255#file255line58>
> >
> >     Please add a comment that these methods aren't actually public, i.e. were made public only to be called from unit tests.
> 
> Oz Linden wrote:
>     A better way to make internals available to a unit test is to make the unit test class a friend of the class it is testing.  This makes the intent clear, and leaves object internals properly hidden.

I don't like this too much for 2 reasons:
1. It requires to forward declare the test classes and makes them a special class having access to all the internal machinery of the tested class. One can argue that it is a good thing but I find this a bit unclean. The unit tests is an opportunity to test the public API of a class in a "context free" way and should stay that way. With that in mind, revisiting a class to put "public" what should be its API with the world is not bad, in that case at least.
2. The whole tut templating machinery makes that declaration actually pretty hard. In truth, I haven't been able to identify one existing unit test doing this.


- Merov


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/63/#review79
-----------------------------------------------------------


On 2010-12-22 23:51:18, Merov Linden wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/63/
> -----------------------------------------------------------
> 
> (Updated 2010-12-22 23:51:18)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> Unit tests addition:
> - add tests for llkdu
> - turned back on and fix unit tests for llimage
> - turned back on and fix unit tests for llworldmap and llworldmipmap
> 
> 
> This addresses bug STORM-744.
>     http://jira.secondlife.com/browse/STORM-744
> 
> 
> Diffs
> -----
> 
>   indra/llimage/CMakeLists.txt 5d69e36a53ee 
>   indra/llimage/tests/llimageworker_test.cpp 5d69e36a53ee 
>   indra/llkdu/CMakeLists.txt 5d69e36a53ee 
>   indra/llkdu/llimagej2ckdu.h 5d69e36a53ee 
>   indra/llkdu/tests/llimagej2ckdu_test.cpp PRE-CREATION 
>   indra/newview/CMakeLists.txt 5d69e36a53ee 
>   indra/newview/tests/llworldmap_test.cpp 5d69e36a53ee 
>   indra/newview/tests/llworldmipmap_test.cpp 5d69e36a53ee 
> 
> Diff: http://codereview.secondlife.com/r/63/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Merov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20101224/61e2b260/attachment.htm