----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39817/#review104815 -----------------------------------------------------------
Thank you for the patch Dian! shell/src/main/java/org/apache/sqoop/shell/utils/ConfigFiller.java (lines 63 - 68) <https://reviews.apache.org/r/39817/#comment163062> I'm wondering what is the purpose of the test flag? Can we drop it? I would much rather to test the class without any special flags to get as close to "production" as possible. shell/src/test/java/org/apache/sqoop/shell/utils/TestConfigFiller.java (lines 87 - 88) <https://reviews.apache.org/r/39817/#comment163059> Can we please use assertTrue and assertFalse instead of using assertEquals comparing to boolean value? Also might I suggest to simply import the static methods on Assert? It will be a bit easier to read. shell/src/test/java/org/apache/sqoop/shell/utils/TestConfigFiller.java (lines 148 - 168) <https://reviews.apache.org/r/39817/#comment163064> We're reusing the inputs quite a lot. Shouldn't we call input.setEmpty() before each separate test (e.g. in adition to call initData()). - Jarek Cecho On Nov. 2, 2015, 3:37 a.m., Dian Fu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39817/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2015, 3:37 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2648 > https://issues.apache.org/jira/browse/SQOOP-2648 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > This JIRA add tests for ConfigFiller. ConfigFiller is used by interactive > mode. > > > Diffs > ----- > > shell/src/main/java/org/apache/sqoop/shell/utils/ConfigFiller.java 6a2a96d > shell/src/test/java/org/apache/sqoop/shell/utils/TestConfigFiller.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/39817/diff/ > > > Testing > ------- > > > Thanks, > > Dian Fu > >
