In interest of trying the new system, I'll dump my local changes back to svn.
First, I made some minor cleanups to the decorator classes and added the ability to get the key for each coordinate (x,y,z) from the XYZDecorator.
Daniel Russel wrote: > In interest of trying the new system, I'll dump my local changes back to > svn. > > First, I made some minor cleanups to the decorator classes and added the > ability to get the key for each coordinate (x,y,z) from the XYZDecorator.
Ah, a perfect demonstration for other eager developers. ;)
1. Please run 'scons standards' before sending patches. This will point out if you have any obvious violations of the style guidelines. Otherwise I have to fix things for you, and I don't like doing that. ;) In this particular case, however, only one minor change was necessary, so I did it for you. This is already in the code review section...
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.
Ben
> > 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. 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.
Daniel Russel wrote: >> 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. > 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.
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
> 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.
How does it help if a unit test specifically calls a particular piece of API as opposed to the API is used internally? In both cases the API is being tested similarly thoroughly.
If I want to change the API, in both cases I change the class and then change some bit of code somewhere calling it. In either case, the the user is screwed if documentation of the change does not make it to them somehow. It only makes a difference if users are monitoring unit tests for evidence of changes. I have trouble imaging anyone doing that given that we have the doxygen docs and a commit list (and eventually, presumably, nice a human readable list of changes).
And given the current state of our unit test code, I hope no user looks at them for anything.
I should add that we do, eventually, want good coverage of our API with example code. However, that coverage just has to be thorough enough so that any any usage can be figured out trivially, no so that any function can be called by cutting a pasting a particular line of code.
It might be worth adding a examples directory at some point. Personally, I would really like it if the examples were built and run as part of the test suite and their output checked, just so that we know they are always working.
On Dec 12, 2007, at 5:23 PM, Daniel Russel wrote:
>> 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. > > How does it help if a unit test specifically calls a particular piece > of API as opposed to the API is used internally? > In both cases the API is being tested similarly thoroughly. > > If I want to change the API, in both cases I change the class and then > change some bit of code somewhere calling it. In either case, the the > user is screwed if documentation of the change does not make it to > them somehow. It only makes a difference if users are monitoring unit > tests for evidence of changes. I have trouble imaging anyone doing > that given that we have the doxygen docs and a commit list (and > eventually, presumably, nice a human readable list of changes). > > And given the current state of our unit test code, I hope no user > looks at them for anything. > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
Daniel Russel wrote: > I should add that we do, eventually, want good coverage of our API with > example code. However, that coverage just has to be thorough enough so > that any any usage can be figured out trivially, no so that any function > can be called by cutting a pasting a particular line of code.
But of course. I'm certainly not suggesting that our unit tests be the only examples. Unit tests and examples are entirely different (we don't even distribute the unit tests for Modeller, for example).
> It might be worth adding a examples directory at some point.
Of course we need an examples directory - there should be no need to even discuss that. But we don't have one yet. (Sure, there were some examples in Bret's initial IMP checkin, but they were clearly designed to be test cases, in which case I put them into the test suite, or they didn't work, in which case I removed them.) When our API is a little more stable, we can think about writing examples.
> Personally, > I would really like it if the examples were built and run as part of the > test suite and their output checked, just so that we know they are > always working.
I don't know if you've ever tried doing that before, but for anything other than really trivial examples, you can't really check the output beyond "did it crash". Look at the Modeller examples, for instance. A given model building example will give dramatically different results on different operating systems (even different compiler releases on the same OS) due to the ruggedness of the scoring function landscape and the limited floating point precision available.
Ben
On Dec 13, 2007, at 11:50 AM, Ben Webb wrote: > I don't know if you've ever tried doing that before, but for anything > other than really trivial examples, you can't really check the output > beyond "did it crash". Sure, but, for the most part, not crashing with asserts enable should be a very strong indication that the code is correct. That is all I meant.
> But of course. I'm certainly not suggesting that our unit tests be the > only examples. Unit tests and examples are entirely different (we > don't > even distribute the unit tests for Modeller, for example). I think we mostly agree. I was just emphasizing that the unit tests should not be examples at all. And hence making sure that each bit of API is called explicitly in the API just adds redundant, nonuseful code and out efforts are best spent elsewhere.
Daniel Russel wrote: > How does it help if a unit test specifically calls a particular piece of > API as opposed to the API is used internally? > In both cases the API is being tested similarly thoroughly.
I think you misunderstood my point. 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). Hiding the test for code in an apparently unrelated patch makes code review hard.
I'm not sure what you mean by "internally" in this context, but if a method is not designed to be used by external users, why is it part of the API?
> And given the current state of our unit test code, I hope no user looks > at them for anything.
That is no excuse for not writing tests, of course, because they can always be incrementally improved. 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).
Ben
participants (2)
-
Ben Webb -
Daniel Russel