[bind10-dev] Coding guidelines and tests

[JINMEI Tatuya / 神明達哉]

At Wed, 12 Oct 2011 17:34:26 +0200,
Tomasz Mrugalski <tomasz at isc.org> wrote:

> Current version of coding guidelines states that "Use 192.0.2.0/24 and
> 2001:db8::/32 for purposes like addresses used in test cases or examples
> in documentation". Documentation purposes is ok, but definitely not tests.
> 
> Tests should be as broad as possible. For example in tests for DHCPv6
> option that conveys IPv6 addresses, I check:
> - :: (any address, a valid value that must be supported),
> - ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff (to see if there are no
> integer overflows),
> - ff02::face:b00c (to see that multicast addresses are handled ok)
> - and some other addresses to verify that tested code does not have
> first 32 bits hardcoded to 2001:db8.
> 
> I propose removing suggestion that 2001:db8::/32 should be used in
> tests. That class is for documentation only, not for tests. The same
> hold true for 192.0.2.0/24.

If I remember correctly, the suggestion of the use of documentation
addresses came from the observation that it should generally be better
to use an address that is very unlikely used by someone's network.  In
some cases our tests accidentally send packets to the test address.
Sending such packets to the wire is itself bad enough (especially for
unit tests, we should actually not rely on such low level system
interfaces in the first place), but it would still be less bad if the
packet is dropped somewhere due to the lack of route.

In the general case, I'm okay with suggesting other addresses than the
documentation addresses if there's any better one that satisfies the
original motivation.  (I know it's not for "tests", and in that sense
we are abusing the notion of them).  But simply insisting not using
them doesn't seem to be good.

In any case, there are some special cases that we need to use special
type of addresses, and in that case this guideline won't apply anyway.

> Another thing that was pointed out in one of ticket reviews is that
> there are more efficient ways of generating test data than plain uint8_t
> array. That is true, but in my opinion more efficient is not always
> suitable for test code. Tests should be as simple (read: error-proof) as
> possible, no necessarily the most efficient. This is not production
> code. I propose to add something along the lines of "In tests,
> simplicity should be favored over efficient, but more complicated code."
> to coding guidelines.

In general that sounds reasonable to me (I don't know the details of
the uint8_t thing, so I have no opinion on that particular case).
BTW, I'd even say that should apply to the main (non test) code: in
general we should prefer simpler and cleaner code than efficient one
(if the efficiency can only be achieved with more complicated code)
unless we really need high performance.

At the risk of splitting hairs, however, I wouldn't use the excuse of
"test is not production code".  IMO, any public code of our source
tree is "in production" in that it indicates the quality of the
software.  If we do a poor job in any part of the code, that will
damage the reliability of the entire code (If I were an outsider and
happened to be interested in the project, I'd browse the source code,
and if any part of it is dirty my trust on that product would be very
much discounted).  So, for example, I'd generally keep any code
including tests exception free unless ensuring that would make the
code *extremely* complicated and unreadable (but in many cases if I
feel I can't make it safe without making it too complicated, that
means my skill isn't just good).

---
JINMEI, Tatuya