sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request: Review request for SQOOP-1035 "Add MS Sqoop Connecter tests to repo"
Date Sat, 15 Jun 2013 17:41:05 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message