> On May 1, 2014, 4:24 p.m., Bill Havanki wrote: > > Hi there! > > > > (I'm new to ZK, and I'm checking out some code reviews to get acquainted > > with the project.) > > > > The unit test here is good, but it doesn't appear to check that the code > > selects the correct set of files to purge, but that the process at least > > completes. It would be good to break out the logic for selecting the files > > into a separate method and then testing that with mock data for the variety > > of snapshot and log files available. It'd be easy to test edge cases that > > way, too. I see that Mockito is already a ZK dependency, and that can > > definitely do it. I'd be happy to help out with creating such a test.
Thank you Bill, its really good idea to test as smaller units. I've tried an approach without using mockito. Could you look at it ? Note: For testing purpose, I've created new method visible for testing PurgeTxnLog#retainNRecentSnapshots() - Rakesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19794/#review41921 ----------------------------------------------------------- On March 29, 2014, 7:23 a.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19794/ > ----------------------------------------------------------- > > (Updated March 29, 2014, 7:23 a.m.) > > > Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and > Camille Fournier. > > > Bugs: ZOOKEEPER-1797 > https://issues.apache.org/jira/browse/ZOOKEEPER-1797 > > > Repository: zookeeper > > > Description > ------- > > PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is > created during this process > > > Diffs > ----- > > ./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1582962 > ./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1582962 > > Diff: https://reviews.apache.org/r/19794/diff/ > > > Testing > ------- > > I've tried to add a tests > > > Thanks, > > Rakesh R > >
