> On Jan. 9, 2013, 8:19 p.m., Cheolsoo Park wrote: > > The changes look good to me. > > > > One minor comment that I had is that it would be helpful for a reviewer if > > you added some comments why the these assertions hold. I was able to > > understand the reason only after reading the loadSubmissions() method in > > DerbyTestCase. > > > > assertTrue(handler.inUseJob(1, getDerbyConnection())); > > assertFalse(handler.inUseJob(2, getDerbyConnection())); > > assertFalse(handler.inUseJob(3, getDerbyConnection())); > > assertFalse(handler.inUseJob(4, getDerbyConnection())); > > > > Adding a simple comment like "// See DerbyTestCase regarding why these > > assertions must hold" will be very helpful. > > > > I don't see many comments in test cases now. I guess we can improve it in > > the future.
Hi Cheolsoo, thank you very much for your review, I appreciate your time. I can't agree with your comment more, so I've created SQOOP-823 to keep that note around. Jarcec - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8803/#review15210 ----------------------------------------------------------- On Jan. 2, 2013, 6:57 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8803/ > ----------------------------------------------------------- > > (Updated Jan. 2, 2013, 6:57 p.m.) > > > Review request for Sqoop. > > > Description > ------- > > I've added proposed functionality. > > > This addresses bug SQOOP-807. > https://issues.apache.org/jira/browse/SQOOP-807 > > > Diffs > ----- > > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > c62730ccd077663c763ece7df0c044acd437ffcc > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectionHandling.java > 4121be74364de33e0cae0aa1fb469a8c7d632922 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java > c0e7a313e887e1fbca244ca92f7cf8b8a1d44bd4 > > Diff: https://reviews.apache.org/r/8803/diff/ > > > Testing > ------- > > I've added unit test + one unit test for similar function with Connection > object. > > > Thanks, > > Jarek Cecho > >
