No reason not to submit. :-)

indra/llkdu/llimagej2ckdu.h (Diff revision 1)

Please add a comment that these methods aren't actually public, i.e. were made public only to be called from unit tests.

1. Oz Linden Dec. 23, 2010, 12:01 p.m. (Dec. 23, 2010, 12:01 p.m.)

   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.

2. Merov Linden Dec. 23, 2010, 4:23 p.m. (Dec. 23, 2010, 4:23 p.m.)

   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.

indra/llkdu/tests/llimagej2ckdu_test.cpp (Diff revision 1)

What about calling protected methods via inheritance?

1. Merov Linden Dec. 23, 2010, 4:18 p.m. (Dec. 23, 2010, 4:18 p.m.)

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

indra/llkdu/tests/llimagej2ckdu_test.cpp (Diff revision 1)

I suppose unit tests should verify something more substantial than not raising exceptions for empty data.

1. Merov Linden Dec. 23, 2010, 4:21 p.m. (Dec. 23, 2010, 4:21 p.m.)

   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.

Huh - I wrote this a long time ago (before the others commented)... apparently I forgot to click on "Publish Review"!

indra/llkdu/llimagej2ckdu.h (Diff revision 1)

This feels wrong. Those functions are implementations of the base class interface, they are called by the base class: if they need to be called directly, something is wrong.

If this is made public because the unit test needs access, then instead add a friend for the unit test class.

1. Aleric Inglewood Dec. 23, 2010, 5:30 p.m. (Dec. 23, 2010, 5:30 p.m.)

   After reading the other comments, I had to add that accessing them by inheritance would definitely be more clean.

2. Cron Stardust Dec. 23, 2010, 9:11 p.m. (Dec. 23, 2010, 9:11 p.m.)

   According to the TUT FAQ (I decided to looking things up as I'm not familiar with TUT itself,) there is no nice way to test private members.  They strongly discourage making the unit test a friend, as it makes the class dependent upon the internals of TUT, possibly causing breakage when TUT is upgraded.  What they DO recommend is to #define both private and protected as public.  That way the source /looks/ like it is supposed to, but winds up being exposed anyway.
   
   Here's the reference: http://tut-framework.sourceforge.net/faq.html#can_we_test_private_methods
   
   Now, this gives me the screaming heebie-jeebies. If this "it says one thing, but does another" #define process is used, it makes the code subject to strange bugs when someone decides to used "protected/private" methods or (shudder) properties directly as if they were supposed to be public...

Thanks, Merov. I have no more objections.

indra/llimagej2coj/llimagej2coj.cpp (Diff revision 2)

Please add the "static" modifier to indicate that the function is not supposed to be used outside of the file.