----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10987/#review21151 -----------------------------------------------------------
Hi Shuaishuai, thank you for this significant effort! I do have couple of high level comments: 1) Thank you for putting the new tests into new "manager" package. Do you think that it would make sense to go even further and put the MS SQL specific tests into "manager/sqlserver" package? I'm assuming that we will add more tests for other connectors in the future as well, so this might help to have the packages clean. 2) Can you register all the new tests in ThirdParty test suite? (https://github.com/apache/sqoop/blob/trunk/src/test/com/cloudera/sqoop/ThirdPartyTests.java) I know that existing SQL Server tests are not there, but I believe that they should. conf/MSTest.properties <https://reviews.apache.org/r/10987/#comment43920> The top level conf/ directory should be used only for code that is mean to be used for the actual product, not for the tests. Would it be feasible to move it somewhere else? For example to testdata/ or src/test? Also this file is missing Apache License. conf/MSTest.properties <https://reviews.apache.org/r/10987/#comment44700> Rest of the project is using lowercased properties separating logical parts using dot. For example JDBC URL is in existing SQLServerManagerImportManualTest specified via following property: sqoop.test.sqlserver.connectstring.host_url I think that it would make sense to stay consistent. Also I would suggest to reuse properties that are already used, such the connection string above. conf/MSTest.properties <https://reviews.apache.org/r/10987/#comment44696> Sqoop is primary supported on linux, so please change all defaults from Windows domains to a linux paths. Also I would strongly advise to use relative paths rather then absolute ones. src/test/com/cloudera/sqoop/manager/SQLServerManagerImportManualTest.java <https://reviews.apache.org/r/10987/#comment44697> Nit: Is this isolated change necessary? src/test/org/apache/sqoop/manager/MSSQLTestData.java <https://reviews.apache.org/r/10987/#comment43921> Can we add javadoc explaining purpose of this class? src/test/org/apache/sqoop/manager/MSSQLTestData.java <https://reviews.apache.org/r/10987/#comment44698> Can we add comments explaining the enum constants with their purpose? src/test/org/apache/sqoop/manager/MSSQLTestData.java <https://reviews.apache.org/r/10987/#comment44699> Nit: Is the main method artifact from previous development? src/test/org/apache/sqoop/manager/MSSQLTestDataFileParser.java <https://reviews.apache.org/r/10987/#comment43922> Nit: This seems to artifact from development. src/test/org/apache/sqoop/manager/MSSQLTestUtils.java <https://reviews.apache.org/r/10987/#comment43923> Is there a linux equivalent for these commands? src/test/org/apache/sqoop/manager/SQLServerDatatypeImportSequenceFileManualTest.java <https://reviews.apache.org/r/10987/#comment43924> Nit: Indentation seems to be off here. testdata/DatatypeTestData-export-lite.txt <https://reviews.apache.org/r/10987/#comment43925> This file do not have a license. We need to either put a license here or configure RAT to skip checking this file. testdata/DatatypeTestData-import-lite.txt <https://reviews.apache.org/r/10987/#comment43926> This file do not have a license. We need to either put a license here or configure RAT to skip checking this file. Jarcec - Jarek Cecho On May 23, 2013, 5:55 p.m., Shuaishuai Nie wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10987/ > ----------------------------------------------------------- > > (Updated May 23, 2013, 5:55 p.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 > >
