----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10987/#review21940 -----------------------------------------------------------
Hi Shuaishuai, I was able to run the tests in my environment and the changes looks good. I do have just couple of nitpicky comments: src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java <https://reviews.apache.org/r/10987/#comment45239> The SQOOP_HOME variable is not defined by default when running tests. I would advise to use some test specific java property, there is many of them already defined or we can define new one if needed The test properties are created in the build.xml file here: https://github.com/apache/sqoop/blob/branch-1.4.0/build.xml#L596 src/test/org/apache/sqoop/manager/sqlserver/ManagerCompatExport.java <https://reviews.apache.org/r/10987/#comment45241> Similar as above, depending on SQOOP_HOME during test execution is not necessary. testdata/MSTest.properties <https://reviews.apache.org/r/10987/#comment45240> The usual workflow on jenkins is to check out the repository and set all required variables and/or properties for running the tests. It's completely fine to store defaults into file, however we have to be able to override all properties during ant execution, e.g something like: ant clean test -Dms.db.server.name=new_host It seems to me that this is not possible with current infrastructure, right? I'm afraid that changing checkout file is not feasible. testdata/MSTest.properties <https://reviews.apache.org/r/10987/#comment45242> Is there a reason why are not reusing the property sqoop.test.sqlserver.connectstring.host_url that is already used in existing SQL Server tests? I would prefer to have one single property for all tests for one single db vendor. Jarcec - Jarek Cecho On June 12, 2013, 5:30 p.m., Shuaishuai Nie wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10987/ > ----------------------------------------------------------- > > (Updated June 12, 2013, 5:30 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 > ----- > > src/test/com/cloudera/sqoop/ThirdPartyTests.java 7e361d2 > src/test/com/cloudera/sqoop/hive/TestHiveImport.java 9c47bad > src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestData.java PRE-CREATION > src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestDataFileParser.java > PRE-CREATION > src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java > PRE-CREATION > src/test/org/apache/sqoop/manager/sqlserver/ManagerCompatExport.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeExportDelimitedFileManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeExportSequenceFileManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportDelimitedFileManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportSequenceFileManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerHiveImportManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerMultiColsManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerMultiMapsManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerParseMethodsManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/sqlserver/SQLServerQueryManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/sqlserver/SQLServerSplitByManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/sqlserver/SQLServerWhereManualTest.java > PRE-CREATION > testdata/DatatypeTestData-export-lite.txt PRE-CREATION > testdata/DatatypeTestData-import-lite.txt PRE-CREATION > testdata/MSTest.properties PRE-CREATION > > Diff: https://reviews.apache.org/r/10987/diff/ > > > Testing > ------- > > All tests passing after applying the patch > > > Thanks, > > Shuaishuai Nie > >
