----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10987/#review20456 -----------------------------------------------------------
Patch looks good except for a few nits. It is a big patch and adds lots of tests which is badly needed. There are few places where the editor generated TODO comments have to be removed and the exception handling can be a little bit more precise (instead of printing the stack trace and ignoring the exception), the exceptions can be rethrown after printing the stackTrace for debugging purposes if debug mode is enabled (you can check the logger) Similar issues on test setup. If the setup throws an exception during one of the methods (say populate the table), it does not handle it - rethrow to abort the test. Similarly getConnection call ignores exception Thanks Venkat src/test/com/cloudera/sqoop/hive/TestHiveImport.java <https://reviews.apache.org/r/10987/#comment42139> Nit - spaces src/test/org/apache/sqoop/manager/MSSQLTestDataFileParser.java <https://reviews.apache.org/r/10987/#comment42140> Can you please remove the comment - it has been implemented src/test/org/apache/sqoop/manager/MSSQLTestDataFileParser.java <https://reviews.apache.org/r/10987/#comment42141> please remove this src/test/org/apache/sqoop/manager/MSSQLTestUtils.java <https://reviews.apache.org/r/10987/#comment42142> Please remove this. Don't we want to return null in this case? src/test/org/apache/sqoop/manager/MSSQLTestUtils.java <https://reviews.apache.org/r/10987/#comment42143> Please remove this exception block. As the method is throwing the SQLException, it should be OK and the caller can handle the exception src/test/org/apache/sqoop/manager/MSSQLTestUtils.java <https://reviews.apache.org/r/10987/#comment42144> Please remove this block or rethrow the exception src/test/org/apache/sqoop/manager/MSSQLTestUtils.java <https://reviews.apache.org/r/10987/#comment42145> Same as before. There are too many of these in the files, can you please address all of them. - Venkat Ranganathan On May 8, 2013, 12:30 a.m., Shuaishuai Nie wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10987/ > ----------------------------------------------------------- > > (Updated May 8, 2013, 12:30 a.m.) > > > Review request for Sqoop. > > > Description > ------- > > Add the MS Sqoop connector tests that test integration scenarios with SQL > Server to the repo. > > > This addresses bug SQOOP-1035. > https://issues.apache.org/jira/browse/SQOOP-1035 > > > Diffs > ----- > > conf/MSTest.properties PRE-CREATION > src/test/com/cloudera/sqoop/hive/TestHiveImport.java 462ccf1 > src/test/com/cloudera/sqoop/manager/SQLServerManagerImportManualTest.java > 27860c2 > src/test/org/apache/sqoop/manager/MSSQLTestData.java PRE-CREATION > src/test/org/apache/sqoop/manager/MSSQLTestDataFileParser.java PRE-CREATION > src/test/org/apache/sqoop/manager/MSSQLTestUtils.java PRE-CREATION > src/test/org/apache/sqoop/manager/ManagerCompatExport.java PRE-CREATION > > src/test/org/apache/sqoop/manager/SQLServerDatatypeExportDelimitedFileManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/SQLServerDatatypeExportSequenceFileManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/SQLServerDatatypeImportDelimitedFileManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/SQLServerDatatypeImportSequenceFileManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/SQLServerHiveImportManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/SQLServerManagerManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/SQLServerMultiColsManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/SQLServerMultiMapsManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/SQLServerParseMethodsManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/SQLServerQueryManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/SQLServerSplitByManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/SQLServerWhereManualTest.java > PRE-CREATION > testdata/DatatypeTestData-export-lite.txt PRE-CREATION > testdata/DatatypeTestData-import-lite.txt PRE-CREATION > > Diff: https://reviews.apache.org/r/10987/diff/ > > > Testing > ------- > > All tests passing after applying the patch > > > Thanks, > > Shuaishuai Nie > >
