Hi Atul,

You are right about test code being very much like app code, so it really should be refactored as well.

One of the reasons that I want all code that gets committed to mainstream to be published as patches here first is that this way everybody will get a chance to look at the patch and comment on it. It is much better to have some people looking at the source code before committing and finding the bugs in advance, rather than have the same people looking for the bug later. I’d like that more people inspect the patches, so that we produce much higher quality code.

Regarding boost and smart pointers, I’ve read a bit about boost though I’ve never used it. Smart pointers is a smart thing but must also be done smartly :-). You should be careful not to use have cycles in your smart pointers and if that is the case anyway, then the loop should be broken by using a weak pointer and that could be tricky sometimes.

I think we should prefer, if applicable of course, to create objects on the stack and pass them by value and rely on reference counting for an efficient implementation. That pointer to the reference counted shared data could be considered “a kind of” smart pointer. If you look at the implementation of Qt you’ll see reference counting all around and you’ll see it does quite a good job there.

There are some cases though where a smart pointer like the one provided by boost would be a smart choice, for example, when an object of a dynamic type should asynchronously be passed as an argument of a signal or slot. For example, when PacketSession emits the packetReceived() signal and the receiver must be in another thread and thus the slot is connected using a Qt::QueuedConnection. This would mean that PacketSession would have to allocate the Packet object on the heap and should rely on the receiver to delete it. But what happens when there are multiple or none slots connected? Then smart pointer would come to the rescue and my opinion is that we should limit their use to only such specific cases. I’m open to hear other opinions as well!

Regards,

Peter

atul wrote:Hey Peter

Thanks for the comments… I will look at the tests once again with your comments ;-)

And yes, you have hit the nail on the head about the leaks… Test code is very much app code — we should take as much care of it as the rest … i.e. it should be refactored and polished often to keep it clean and glowing….

About the leaks, what about using boost? Especially Boost smart pointers… Boost is a brilliant library written by C++ gurus so we could as well benefit from it…
I have some experience of using boost … so we can as well use it in the test code..

—- cheers atul

Peter Dimov wrote:

Hi Atul,

That’s a neat trick to derive a class Bye in order to make all protected functions public and be able to test them :-). Is that what you were referring to by “circumventing around private declarations”? You can actually only change the visibility of protected declarations. Is there really a trick for private declarations?

I think one should be careful when testing Bye::copy() since the function returns a pointer to an object created on the heap. If it is not deleted we’ll have memory leaks in the test application. I think it would be quite wise to run Valgrind on the test app first and then on Calitko. If a unit tests leaks memory, we would be able to much easier test and solve the leak. Perfectly, if we have good test coverage and have no leaks during testing, then we should not have leaks in Calitko :-).

Some comments about some of the tests:

testInvalidate() – the header is invalidated when a field is modified by calling a setter, or when the payload is invalidated (a payload field is modified by calling a setter). The reference counted shared data is then copied and are flags being set, so that next time rawData() or rawPayload() are called the raw buffers be rebuilt. If a packet is merely forwarded by our node, only the header will be modified and rebuilt but not the payload. It is a bad idea to rebuild the payloads of transiting packets.

testPrepareReadPayloadChecks() - the raw format of the packet payload is: 2 bytes status code in little endian followed by a zero terminated string. So your first check raw payload “H” fails because a correct packet would contain at least 2 bytes for code and a zero. The second one works but the message is not “Hello”. I think you should also check whether the code gets parsed correctly as well.

testPrepareWritePayload() you might also set the status code an compare the parsed data with the binary data.

testReadHeader() – The last assertion fails because QDataStream uses by default big endian and Gnutella packets use little endian. The bytes in the read payloadLength are swaped.

This last test shows an interesting concept – writing test code that can be run on different data. That’ll be pretty important and QTestLib has this concept, although I don’t feel their realization very practical. For each unit test function you can declare a _data function which fills in a table where rows are tests and columns are test data. I couldn’t find anything like that in CppUnit. Does anybody have any idea whether something like that might work in CppUnit. It would also be quite interesting to be able to read the data from files. That would make the source code more clear maybe.

Regards,

Peter

atul wrote:Dear all

Here are some of the test cases I came up with…

Bye::testReadHeader() is pretty interesting and I believe I learned a lot by writing and making it work ;-)
I am looking into why the last test assertion is failing ;-)
I have taken the technique of circumventing around private declarations from Mike Feather’s book …

— cheers atul