Thomas, we all agree that you have done useful work. But right now it seems that you are more concerned with the aesthetics of the code base than the correctness of it. You are capable of identifying potential bugs. Are you capable of doing the work to provably identify them (by writing tests) and fix them?
Now that you've drawn my attention to this, I have some questions. How the heck are ZOOKEEPER-1265 and ZOOKEEPER-1247 "bugfixes" if you fixed them without putting in a single test? That is not a bugfix. Nothing about those changes precluded writing tests for them, or modifying existing tests to prove the fix. It seems like perhaps they were viewed as refactoring, because otherwise I can't imagine why they weren't tested. You don't fix a bug unless you have a test that shows the bug by failing before the change and passing afterwards. Period. Please go back and write tests for these two fixes, otherwise I may revert the checkins and reopen the tickets. Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly. C -----Original Message----- From: Thomas Koch [mailto:[email protected]] Sent: Tuesday, November 01, 2011 12:12 PM To: [email protected] Cc: Fournier, Camille F. [Tech]; 'Benjamin Reed' Subject: Re: cleanup and subjective patches Fournier, Camille F.: > Any changes coming in without tests should really be meaningfully > untestable. I completely agree with the suggestion to require a testing > uplift if you want to add refactorings unless you know the refactored code > has 90+% test coverage. > > Personally, I have no problems with refactorings, but we seem to be > spending much of our time dealing with them right now when we really need > to get 3.4 out the door. It's been months that this release has been going > on and splitting the committer attention between 3.4 and refactoring > changes feels counterproductive to the needs of the community at this > moment. > > C Some facts for the last two months: 25 patches submitted by me were committed added lines: 1805 deleted lines: 5795 ===== -3990 Two patches were about big legacy code removals, excluding them: added lines: 1770 deleted lines: 2013 ===== -243 Critical blocker bugs found because of doing refactorings: ZOOKEEPER-1269 Multi deserialization issues ZOOKEEPER-1246 Dead code in PrepRequestProcessor catch Exception block And two other possible issues described in mail "Possible failure scenarios with deserialization + multi?" minor bugs found: ZOOKEEPER-1247 dead code in PrepRequestProcessor.pRequest multi case ZOOKEEPER-1272 ZooKeeper.multi() could violate API if server misbehaves ZOOKEEPER-1265 Normalize switch cases lists on request types functional improvements: ZOOKEEPER-1175 DataNode references parent node for no reason ZOOKEEPER-556 Startup messages should account for common error of missing leading slash in config files missing tests added: ZOOKEEPER-1254 test correct watch handling with multi ops ZOOKEEPER-1259_central_mapping_from_type_to_txn_record_class (not yet committed) At the beginning of October I also reviewed _all_ open bugs and closed a couple of obsolete onces or asked for feedback. Regards, Thomas Koch, http://www.koch.ro
