Scott Gray wrote: > On 5/04/2010, at 9:43 PM, Bob Morley wrote: > >> Is it reasonable for a non-commiter to review commits? To be honest, I never >> even considered that I should be doing that. > > Reasonable? It would be fantastic, we have 166 subscribers and very little > feedback.
More eyes, the better. Tell us where we are wrong, tell us what could be done better. >> However, two other things occurred to me ... >> >> Got the distinct feeling that this points to a not very good practice when >> it comes to unit testers. This wasn't a problem of not having tests. If I had ran the test cases, even if I had done a full compile, I would found this particular error before I ever committed it. And for that, I can't apologize enough for. I've been adding lots of unit tests to base. You'll find a bunch of my handiwork in classes that have the SourceMonitored annotation. > Buildbot has been reporting failures since yesterday: > http://ci.apache.org/waterfall?show=ofbiz-trunk > The problem wasn't missed, I just think Adam was a little peeved that no one > noticed his simple mistake in such as small commit. I happily admit to causing a fuckup. That wasn't the point I was making, as you surmised. But, if no one is reviewing commits, and such a simple, obvious error wasn't detected by someone else, then how can we even detect the more complex problems? I understand we are all human, and that we are all busy. I've been quite good at finding and tracking down buildbot problems before(git bisect, woo!). I'm just annoyed that no one else pointed the finger at me, and that it sat for a day. >> I definitely understand the pain of attempting to >> force a "must have tester" policy but having a project code-coverage goal >> might be something to strive towards (with published metrics). Moreover, I >> would guess in this case a unit-tester would have likely caught the inverted >> condition check (if it was compiling). Install cobertura.jar into framework/base/lib, compile, and you will get metrics. It's just that cobertura is gpl, and there are no other good tools that are currently being maintained that are also license compatible. I will *not* spend any of my time working at integrating a non-free coverage library. Period. Don't even bother asking me. I have written the cobertura integration has a pluggable system. >> I am definitely not a fisheye expert, but I would hope with its JIRA >> integration that something could be automated that would drive reviews >> targeted at the commiters. Again having a code-review goal (published) >> would be something the project could strive towards. >> >> It would be pretty awesome to have Ofbiz - 2+ million lines of code, 100% >> code reviewed, 90% unit-test code coverage. > > This isn't a dig at you, but talk is cheap :-) > Write some tests and I'll commit them. Or I.
