Daniel Russel wrote:
Ahhh, but I cleverly leveraged the existing test cases to run through the new code in the restraints patch :-) Much better than adding more code to the test suite.2. This also points out something which is not in the code review section, but which I'll add now. If you add new methods/classes, you should add unit tests for them. Even simple tests which just stupidly call each of the new methods are better than nothing. Unfortunately Subversion doesn't force you to commit tests along with code, so we will have to rely on policy for this.
In that case, I have either misunderstood your first patch (which claims to add a new method) or the existing test cases are not really unit tests. Unit tests should be on the smallest practical units, otherwise your methods can change without breaking the tests while some poor user using your methods will have their code break.
Ben
--
http://salilab.org/~ben/
"It is a capital mistake to theorize before one has data."
- Sir Arthur Conan Doyle