[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [IMP-dev] First code review



Daniel Russel wrote:
On Dec 13, 2007, at 11:41 AM, Ben Webb wrote:
Coverage is one thing, but a simple
unit test that directly accompanies a new piece of code helps to
document the intent of that code (e.g. in a submitted patch).

But unit tests are not documentation
(sure, it'd be nice if they were readable enough to serve as simple
examples, but tests and examples are two rather different things).

You above say that unit tests help to document the intent and below say that they are not documentation. I agree with the later and think trying to do it with the former just creates excess code with no purpose.

I don't see any inconsistency there - we're documenting different things. Unit tests are not documentation in themselves, but do provide some indication of the intent of a method which is useful in code review (particularly if the accompanying description of the patch is lacking or the doxygen comments are incomplete). I'm simply talking about test-driven coding here. Documentation is great for saying "foo_method takes a positive integer and returns its square, or RangeError on error" but a unit test that invokes that method and asserts that RangeError is thrown for a negative integer actually enforces that. That's not "excess code with no purpose" because the documentation does not ensure that happens.

As a side note, having the test code in an unrelated patch is generally a bad idea,

I don't know why you say "as a side note", because that's the point I'm making here.

but separating out changes into clean patches is not always easy and in this case would have involved significantly more work.

Sure, which is why it's important to have a code review policy in mind before writing such patches.

	Ben
--
                      http://salilab.org/~ben/
"It is a capital mistake to theorize before one has data."
	- Sir Arthur Conan Doyle