Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Boglarka Egyed]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review210545
---


Ship it!




Hi Nguyen,

Thank you very much for the update! Now it looks good to me, let's ship it! :)

Thanks,
Bogi

- Boglarka Egyed


On Nov. 13, 2018, 6:18 a.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Nov. 13, 2018, 6:18 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle 2014b5cf5 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/HBaseTestCase.java 94b71b61c 
>   src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java 65f079467 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 
> 276e9eaa4 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
>   src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
> f1768ee76 
>   src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
> ff13dc3bc 
>   src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
>   src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
>   src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
>   src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
>   src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
>   src/test/org/apache/sqoop/lib/TestBooleanParser.java 

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Szabolcs Vasas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review210541
---


Ship it!




Hi Nguyen,

Thank you for updating your patch, I ran the unit and third party tests and 
everything is green.

- Szabolcs Vasas


On Nov. 13, 2018, 6:18 a.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Nov. 13, 2018, 6:18 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle 2014b5cf5 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/HBaseTestCase.java 94b71b61c 
>   src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java 65f079467 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 
> 276e9eaa4 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
>   src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
> f1768ee76 
>   src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
> ff13dc3bc 
>   src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
>   src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
>   src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
>   src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
>   src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
>   

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Nguyen Truong]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/
---

(Updated Nov. 13, 2018, 6:18 a.m.)


Review request for Sqoop.


Changes
---

Hi all,

Thank you for your patience.

Besides categories, I also changed a line in 
org.apache.sqoop.hbase.HBaseTestCase#setUp according to Szabolc's suggestion as 
it caused another test interdependency.

I have tested the patch with ./gradlew test.

Thank you,
Natalie


Bugs: SQOOP-3104
https://issues.apache.org/jira/browse/SQOOP-3104


Repository: sqoop-trunk


Description
---

We are currently unsing test naming conventions to differentiate between 
ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
junit categories which will allow us to have more categories in the future. 
This would also remove the reliance on the test class name.

Test categories skeleton:
  SqoopTest _ UnitTest
  |__ IntegrationTest
  |__ ManualTest

  ThirdPartyTest _ CubridTest
   |__ Db2Test
   |__ MainFrameTest
   |__ MysqlTest
   |__ NetezzaTest
   |__ OracleTest
   |__ PostgresqlTest
   |__ SqlServerTest

  KerberizedTest

Categories explanation:
* SqoopTest: Group of the big categories, including:
- UnitTest: It tests one class only with its dependencies mocked or if 
the dependency
is lightweight we can keep it. It must not start a minicluster or an 
hsqldb database.
It does not need JCDB drivers.
- IntegrationTest: It usually tests a whole scenario. It may start up 
miniclusters,
hsqldb and connect to external resources like RDBMSs.
- ManualTest: This should be a deprecated category which should not be 
used in the future.
It only exists to mark the currently existing manual tests.
* ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC driver 
and/or a docker
container/external RDBMS instance to run. Subcategories express what kind 
of external
resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
driver on the classpath
* KerberizedTest: Test that needs Kerberos, which needs to be run on a 
separate JVM.

Opinions are very welcomed. Thanks!


Diffs (updated)
-

  build.gradle 2014b5cf5 
  src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
  src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
  src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
  src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
  src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
f5700ce65 
  src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
244831672 
  
src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
 d3e3fb23e 
  src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
  src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
3bfb39178 
  src/test/org/apache/sqoop/hbase/HBaseTestCase.java 94b71b61c 
  src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
  src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
  src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
  
src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java 
4d2cb2f88 
  src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
  src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
  src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java 65f079467 
  src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
  src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 276e9eaa4 
  src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
  src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
f1768ee76 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3bc 
  src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
  src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
  src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
  src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
  src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
  src/test/org/apache/sqoop/lib/TestBooleanParser.java 914ab37e4 
  src/test/org/apache/sqoop/lib/TestClobRef.java f94d1a8af 
  src/test/org/apache/sqoop/lib/TestFieldFormatter.java 9ac55e703 
  src/test/org/apache/sqoop/lib/TestLargeObjectLoader.java 1e07d7174 
  src/test/org/apache/sqoop/lib/TestRecordParser.java d6844c1cf 
  src/test/org/apache/sqoop/manager/TestDefaultManagerFactory.java 

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Szabolcs Vasas]

> On Sept. 10, 2018, 8:55 a.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/manager/oracle/OraOopTestCase.java
> > Lines 55 (patched)
> > 
> >
> > This class should be an IntegrationTest too.

Sorry, my comment was misleanding here, I meant that OraOopTestCase should have 
both OracleTest and IntegrationTest categories.


> On Sept. 10, 2018, 8:55 a.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerTest.java
> > Lines 73 (patched)
> > 
> >
> > This test should be an integrationTest too.

Sorry, my comment was misleanding here, I meant that OraOopTestCase should have 
both OracleTest and IntegrationTest categories.


- Szabolcs


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208478
---


On Sept. 23, 2018, 2:01 a.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Sept. 23, 2018, 2:01 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 
> 

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Szabolcs Vasas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review209452
---



Hi Natalie,

Thank you for updating your patch, sorry for the misleading comments, please 
see my reply on them.
Once you make this change I think we can go ahead and commit this.

Regards,
Szabolcs

- Szabolcs Vasas


On Sept. 23, 2018, 2:01 a.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Sept. 23, 2018, 2:01 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 
> 276e9eaa4 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
>   src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
> f1768ee76 
>   src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
> ff13dc3bc 
>   src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
>   src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
>   src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
>   src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
>   src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
>   

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Boglarka Egyed]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review209262
---



Hi Nguyen,

Thank you so much for the patch update and sorry for the late review!

I ran ./gradlew test but many tests with integrationPlainTest category failed 
for me because they were missing the JDBC driver. So should I pass them via the 
-Dsqoop.thirdparty.lib.dir= option? 
This is not clear to me and I couldn't find it in the description either.

Also, where can I find some guidelines (command examples) about how to run 
specific categories?

Thank you in advance,
Bogi

- Boglarka Egyed


On Sept. 23, 2018, 2:01 a.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Sept. 23, 2018, 2:01 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 
> 276e9eaa4 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
>   src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
> f1768ee76 
>   src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
> ff13dc3bc 
>   src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
>   

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Fero Szabo]

Hey Natalie,

Kudos, for working on this even on the weekend!

All of us appreciate the effort!

Cheers,
Fero

On Sat, Sep 22, 2018 at 7:01 PM Nguyen Truong 
wrote:

>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
>
> (Updated Sept. 23, 2018, 2:01 a.m.)
>
>
> Review request for Sqoop.
>
>
> Changes
> ---
>
> Hi all,
> Thank you very much for your review and detailed suggestion.
> Sorry, it took me forever to update this diff.
> Best,
> Natalie
>
>
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
>
>
> Repository: sqoop-trunk
>
>
> Description
> ---
>
> We are currently unsing test naming conventions to differentiate between
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented
> junit categories which will allow us to have more categories in the future.
> This would also remove the reliance on the test class name.
>
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
>
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
>
>   KerberizedTest
>
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked
> or if the dependency
> is lightweight we can keep it. It must not start a minicluster or
> an hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start
> up miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should
> not be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what
> kind of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and
> Oracle driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a
> separate JVM.
>
> Opinions are very welcomed. Thanks!
>
>
> Diffs (updated)
> -
>
>   build.gradle fc7fc0c4c
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java
> f5700ce65
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java
> 244831672
>
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
> d3e3fb23e
>   src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java
> 3bfb39178
>   src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb
>
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
> 4d2cb2f88
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java
> b55179a4f
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java
> 276e9eaa4
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6
>   src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java
> f1768ee76
>
> src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java
> ff13dc3bc
>   src/test/org/apache/sqoop/io/TestCodecMap.java e71921823
>   src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283
>   src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08
>   src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad
>   src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b
>   src/test/org/apache/sqoop/lib/TestBooleanParser.java 914ab37e4
>   src/test/org/apache/sqoop/lib/TestClobRef.java f94d1a8af
>   src/test/org/apache/sqoop/lib/TestFieldFormatter.java 9ac55e703
>   src/test/org/apache/sqoop/lib/TestLargeObjectLoader.java 1e07d7174
>   

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Nguyen Truong]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/
---

(Updated Sept. 23, 2018, 2:01 a.m.)


Review request for Sqoop.


Changes
---

Hi all,
Thank you very much for your review and detailed suggestion.
Sorry, it took me forever to update this diff.
Best,
Natalie


Bugs: SQOOP-3104
https://issues.apache.org/jira/browse/SQOOP-3104


Repository: sqoop-trunk


Description
---

We are currently unsing test naming conventions to differentiate between 
ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
junit categories which will allow us to have more categories in the future. 
This would also remove the reliance on the test class name.

Test categories skeleton:
  SqoopTest _ UnitTest
  |__ IntegrationTest
  |__ ManualTest

  ThirdPartyTest _ CubridTest
   |__ Db2Test
   |__ MainFrameTest
   |__ MysqlTest
   |__ NetezzaTest
   |__ OracleTest
   |__ PostgresqlTest
   |__ SqlServerTest

  KerberizedTest

Categories explanation:
* SqoopTest: Group of the big categories, including:
- UnitTest: It tests one class only with its dependencies mocked or if 
the dependency
is lightweight we can keep it. It must not start a minicluster or an 
hsqldb database.
It does not need JCDB drivers.
- IntegrationTest: It usually tests a whole scenario. It may start up 
miniclusters,
hsqldb and connect to external resources like RDBMSs.
- ManualTest: This should be a deprecated category which should not be 
used in the future.
It only exists to mark the currently existing manual tests.
* ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC driver 
and/or a docker
container/external RDBMS instance to run. Subcategories express what kind 
of external
resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
driver on the classpath
* KerberizedTest: Test that needs Kerberos, which needs to be run on a 
separate JVM.

Opinions are very welcomed. Thanks!


Diffs (updated)
-

  build.gradle fc7fc0c4c 
  src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
  src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
  src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
  src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
  src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
f5700ce65 
  src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
244831672 
  
src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
 d3e3fb23e 
  src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
  src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
3bfb39178 
  src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
  src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
  src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
  
src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java 
4d2cb2f88 
  src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
  src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
  src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
  src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
  src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 276e9eaa4 
  src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
  src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
f1768ee76 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3bc 
  src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
  src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
  src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
  src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
  src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
  src/test/org/apache/sqoop/lib/TestBooleanParser.java 914ab37e4 
  src/test/org/apache/sqoop/lib/TestClobRef.java f94d1a8af 
  src/test/org/apache/sqoop/lib/TestFieldFormatter.java 9ac55e703 
  src/test/org/apache/sqoop/lib/TestLargeObjectLoader.java 1e07d7174 
  src/test/org/apache/sqoop/lib/TestRecordParser.java d6844c1cf 
  src/test/org/apache/sqoop/manager/TestDefaultManagerFactory.java 8e1632430 
  src/test/org/apache/sqoop/manager/TestSqlManager.java 185f5a7a1 
  src/test/org/apache/sqoop/manager/cubrid/CubridAuthTest.java 82fac12e3 
  

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Boglarka Egyed]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208481
---



Hi Nguyen,

Many thanks for updating the patch, I have one last command regarding the 
documentation part, please find it below.

Thank you,
Bogi


src/docs/dev/test-categories-definitions.txt
Lines 1 (patched)


This file should have been included into 
sqoop/src/docs/dev/SqoopDevGuide.txt so that ant docs and gradle docs generate 
it to the the SqoopDevGuide html too.
However, after thinking it through, I think it makes no sense to have these 
definitions written here and in the interface's javadocs too as it would mean 
duplicated documentation that will be failed to maintain parallel at some point 
in the future.
So I would rather suggest to:
- keep the definitions in the javadocs, as you have already added them in 
your new patch,
- remove this (test-categories-definitions.txt) file.


- Boglarka Egyed


On Sept. 8, 2018, 2:57 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Sept. 8, 2018, 2:57 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/docs/dev/test-categories-definitions.txt PRE-CREATION 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
>   

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Szabolcs Vasas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208478
---



Hi Nguyen,

Thank you for improving the patch!
I have executed the tests (unitTest, integrationPlainTest, kerberizedTest, 
test, thirdPartyTest, sqoopTest tasks) and everything works fine on my side.
I think the issue could be that the test times out on your side, try increasing 
the timeout value in the @Test annotation on testWideTableClassGeneration test.
I haven't managed to reproduce the failure even when I executed the tests in 
the order you provided.

re:
Changed the JacocoTestReport into a task. Please take a look at this. I am not 
sure I understood it correctly

It is going to be a bit more complicated, we will need to override the 
executionData field to a value other than test, but we will take care of this 
in a next patch, you don't have to worry about this now. 

re:
Changed dependsOn to finalizedBy for sqoopTest and test so all tests will run, 
even when some others fail. If I understand it correctly, if we use dependsOn, 
a test task won't run if the one before it failed. Also, I need your opinion on 
this.

Yes, this is correct, thank you!

I have left a few minor findings, once you fix those I think we will be good to 
go!

Thanks,
Szabolcs


build.gradle
Lines 188 (patched)


I think we should add some documentation here explaining why we still use 
the include field with includeCategories. Something like this would be useful:
"A Gradle test task with forkEvery 1 and includeCategories performs poorly 
because it starts a new JVM even for the filtered tests.
To work around this performance problem we need to add every kerberized 
test in a separate include field here."



src/test/org/apache/sqoop/manager/TestMainframeManager.java
Lines 55 (patched)


This test should not be a MainFrameTest since it does not require an 
external instance.



src/test/org/apache/sqoop/manager/oracle/OraOopTestCase.java
Lines 55 (patched)


This class should be an IntegrationTest too.



src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java
Lines 80 (patched)


This test should be annotated with has to be annotated with 
@Parameterized.UseParametersRunnerFactory(BlockJUnit4ClassRunnerWithParametersFactory.class)



src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerTest.java
Lines 73 (patched)


This test should be an integrationTest too.



src/test/org/apache/sqoop/util/BlockJUnit4ClassRunnerWithParameters.java
Lines 1 (patched)


Please add license header to this new file.



src/test/org/apache/sqoop/util/BlockJUnit4ClassRunnerWithParametersFactory.java
Lines 1 (patched)


Please add license header to this new file.



src/test/org/apache/sqoop/util/BlockJUnit4ClassRunnerWithParametersFactory.java
Lines 8 (patched)


It would be good to add documentation to this class to clarify why it is 
needed. Something like:
"Due to an issue in JUnit Gradle does not execute categorized tests which 
are parameterized too.
The issue is already 
reported(https://github.com/junit-team/junit4/issues/751) but its fix will be 
released in JUnit 4.13.
This factory returns a custom BlockJUnit4ClassRunnerWithParameters 
instance which contains the fix for this issue so we have to use 
@Parameterized.UseParametersRunnerFactory(BlockJUnit4ClassRunnerWithParametersFactory.class)
 annotation on parameterized test classes until JUnit 4.13 is released and we 
migrate to it."


- Szabolcs Vasas


On Sept. 8, 2018, 2:57 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Sept. 8, 2018, 2:57 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ 

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Nguyen Truong]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/
---

(Updated Sept. 8, 2018, 2:57 p.m.)


Review request for Sqoop.


Changes
---

I think this is another interdependency resulted by executing them in a 
different order. This is the test order in my machine when I ran test. Thanks.


Bugs: SQOOP-3104
https://issues.apache.org/jira/browse/SQOOP-3104


Repository: sqoop-trunk


Description
---

We are currently unsing test naming conventions to differentiate between 
ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
junit categories which will allow us to have more categories in the future. 
This would also remove the reliance on the test class name.

Test categories skeleton:
  SqoopTest _ UnitTest
  |__ IntegrationTest
  |__ ManualTest

  ThirdPartyTest _ CubridTest
   |__ Db2Test
   |__ MainFrameTest
   |__ MysqlTest
   |__ NetezzaTest
   |__ OracleTest
   |__ PostgresqlTest
   |__ SqlServerTest

  KerberizedTest

Categories explanation:
* SqoopTest: Group of the big categories, including:
- UnitTest: It tests one class only with its dependencies mocked or if 
the dependency
is lightweight we can keep it. It must not start a minicluster or an 
hsqldb database.
It does not need JCDB drivers.
- IntegrationTest: It usually tests a whole scenario. It may start up 
miniclusters,
hsqldb and connect to external resources like RDBMSs.
- ManualTest: This should be a deprecated category which should not be 
used in the future.
It only exists to mark the currently existing manual tests.
* ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC driver 
and/or a docker
container/external RDBMS instance to run. Subcategories express what kind 
of external
resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
driver on the classpath
* KerberizedTest: Test that needs Kerberos, which needs to be run on a 
separate JVM.

Opinions are very welcomed. Thanks!


Diffs
-

  build.gradle fc7fc0c4c 
  src/docs/dev/test-categories-definitions.txt PRE-CREATION 
  src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
  src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
  src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
  src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
  src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
f5700ce65 
  src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
244831672 
  
src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
 d3e3fb23e 
  src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
  src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
3bfb39178 
  src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
  src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
  src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
  
src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java 
4d2cb2f88 
  src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
  src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
  src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
  src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
  src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 276e9eaa4 
  src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
  src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
f1768ee76 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3bc 
  src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
  src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
  src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
  src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
  src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
  src/test/org/apache/sqoop/lib/TestBooleanParser.java 914ab37e4 
  src/test/org/apache/sqoop/lib/TestClobRef.java f94d1a8af 
  src/test/org/apache/sqoop/lib/TestFieldFormatter.java 9ac55e703 
  src/test/org/apache/sqoop/lib/TestLargeObjectLoader.java 1e07d7174 
  src/test/org/apache/sqoop/lib/TestRecordParser.java d6844c1cf 
  src/test/org/apache/sqoop/manager/TestDefaultManagerFactory.java 8e1632430 
  src/test/org/apache/sqoop/manager/TestMainframeManager.java c84f05f66 
  

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Nguyen Truong]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/
---

(Updated Sept. 8, 2018, 3:47 a.m.)


Review request for Sqoop.


Changes
---

- Applied Szabolcs' fix on parameterized issue
- Added categories for missing tests
- Changed the JacocoTestReport into a task. Please take a look at this. I am 
not sure I understood it correctly
- Changed dependsOn to finalizedBy for sqoopTest and test so all tests will 
run, even when some others fail. If I understand it correctly, if we use 
dependsOn, a test task won't run if the one before it failed. Also, I need your 
opinion on this.
- Added definitions into a file under sqoop/src/docs/dev and also javadoc at 
each category interface
Thank you very much for your help and opinion.
I ran ./gradlew test and one test failed. That was TestClassWriter > 
testWideTableClassGeneration. It was a connection refused error. I also need 
your help with this.
Thanks :)


Bugs: SQOOP-3104
https://issues.apache.org/jira/browse/SQOOP-3104


Repository: sqoop-trunk


Description
---

We are currently unsing test naming conventions to differentiate between 
ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
junit categories which will allow us to have more categories in the future. 
This would also remove the reliance on the test class name.

Test categories skeleton:
  SqoopTest _ UnitTest
  |__ IntegrationTest
  |__ ManualTest

  ThirdPartyTest _ CubridTest
   |__ Db2Test
   |__ MainFrameTest
   |__ MysqlTest
   |__ NetezzaTest
   |__ OracleTest
   |__ PostgresqlTest
   |__ SqlServerTest

  KerberizedTest

Categories explanation:
* SqoopTest: Group of the big categories, including:
- UnitTest: It tests one class only with its dependencies mocked or if 
the dependency
is lightweight we can keep it. It must not start a minicluster or an 
hsqldb database.
It does not need JCDB drivers.
- IntegrationTest: It usually tests a whole scenario. It may start up 
miniclusters,
hsqldb and connect to external resources like RDBMSs.
- ManualTest: This should be a deprecated category which should not be 
used in the future.
It only exists to mark the currently existing manual tests.
* ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC driver 
and/or a docker
container/external RDBMS instance to run. Subcategories express what kind 
of external
resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
driver on the classpath
* KerberizedTest: Test that needs Kerberos, which needs to be run on a 
separate JVM.

Opinions are very welcomed. Thanks!


Diffs (updated)
-

  build.gradle fc7fc0c4c 
  src/docs/dev/test-categories-definitions.txt PRE-CREATION 
  src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
  src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
  src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
  src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
  src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
f5700ce65 
  src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
244831672 
  
src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
 d3e3fb23e 
  src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
  src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
3bfb39178 
  src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
  src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
  src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
  
src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java 
4d2cb2f88 
  src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
  src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
  src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
  src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
  src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 276e9eaa4 
  src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
  src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
f1768ee76 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3bc 
  src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
  src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
  src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
  

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Boglarka Egyed]

> On Aug. 31, 2018, 1:24 p.m., Boglarka Egyed wrote:
> > Hi Nguyen,
> > 
> > Many thanks for taking care of this huge effort! Having well categorized 
> > tests is a long-awaited improvement Sqoop, it will ease the life of every 
> > developer.
> > 
> > In general your change looks good to me however I have three findings:
> > 
> > 1.) New test classes have been introduced recently in SQOOP-3348, 
> > SQOOP-3363 and SQOOP-3368 testing the Sqoop-S3 integration. I think it 
> > would make sense to have a new test category for the S3 related tests 
> > similarly to the KerberizedTest category.
> > 
> > 2.) There should be well defined, thorough definitions of each test 
> > categories and these should be well documented somewhere. This is something 
> > that could be a result of a discussion amongst the contributors. After 
> > creating such definitions they could be added as javadocs to the test 
> > category interfaces as well as included in the Sqoop Developer’s Guide 
> > (changes would be needed under sqoop/src/docs/dev).
> > 
> > 3.) It would be also very useful to have exact commands described to run 
> > specific categories in the COMPILING.txt.
> > 
> > Thank you very much,
> > Bogi
> 
> Nguyen Truong wrote:
> Hi Boglarka,
> 
> Thanks for your review. I will go on and add categories for the new tests.
> 
> I don't know about the S3 tests. Could you please let me know more why 
> they should be under a separate category as KerberizedTest? Thank you very 
> much.
> 
> I will update the COMPILING.txt after we come up with the final decision 
> on the categories.
> 
> Thanks,
> Nguyen
> 
> Boglarka Egyed wrote:
> S3 tests should be in a separate test category as they test the 
> integration with the Amazon S3 cloud service that is a third party side just 
> like an RDBMS. These tests also require AWS credentials to access S3 and they 
> run only if these credentials are provided via the 
> -Ds3.generator.command= property as well as the 
> target S3 location via the -Ds3.bucket.url= property.
> The S3 tests that should be in a separate category are under 
> sqoop/src/test/org/apache/sqoop/s3/
> The S3 related test case added in SQOOP-3368 is a simple unit test case 
> though, it doesn't test the integration with S3 just the sqoop option 
> validation mechanism.
> 
> I will also think about the definitions of the test categories and will 
> share my thoughts later.

Hey Natalie,

I think I ran a bit forward with my previous comments. Let's skip the S3 
category now, it can be added in a later patch as well. Please ignore my words 
above.

We had a discussion with Szabolcs and Fero and here is our recommendation for 
the test categorie definitions:

SqoopTest
-
UnitTest:
A unit test shall test one class at a time having it's dependencies mocked.
A unit test shall not start a mini cluster nor an embedded database and it 
shall not use a JDBC driver.

IntegrationTest:
An integration test shall test if independently developed classes work together 
correctly.
An integration test checks a whole scenario and thus may start mini clusters or 
embedded databases and may connect to external resources like RDBMS instances.

ManualTest:
Deprecated category, shall not be used nor extended.

ThirdPartyTest
--
A third party test shall test a scenario where a third party side is required 
such as a JDBC driver or an external RDBMS instance.

CubridTest:
A CubridTest shall test scenarios where a Cubrid driver and/or external 
instance is required.

Db2Test:
A DB2 test shall test scenarios where a DB2 driver and/or external instance is 
required.

MainFrameTest:
A MainFrame test shall test scenarios where a MainFrame external instance is 
required.

MysqlTest:
A MySql test shall test scenarios where a MySql driver and/or external instance 
is required.

NetezzaTest:
A Netezza test shall test scenarios where a Netezza driver and/or external 
instance is required.

OracleTest:
An Oracle test shall test scenarios where a Oracle driver and/or external 
instance is required.

PostgresqlTest:
A PostgreSql test shall test scenarios where a PostgreSql driver and/or 
external instance is required.

SqlServerTest:
An SqlServer test shall test scenarios where a SqlServer driver and/or external 
instance is required.

KerberizedTest
--
A kerberized test shall run in kerberized environment thus it starts mini KDC 
server.


These also can be refined and/or extended later but could be a good start for 
now.

What do you think?

Thanks,
Bogi


- Boglarka


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208172
---


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To 

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Boglarka Egyed]

> On Aug. 31, 2018, 1:24 p.m., Boglarka Egyed wrote:
> > Hi Nguyen,
> > 
> > Many thanks for taking care of this huge effort! Having well categorized 
> > tests is a long-awaited improvement Sqoop, it will ease the life of every 
> > developer.
> > 
> > In general your change looks good to me however I have three findings:
> > 
> > 1.) New test classes have been introduced recently in SQOOP-3348, 
> > SQOOP-3363 and SQOOP-3368 testing the Sqoop-S3 integration. I think it 
> > would make sense to have a new test category for the S3 related tests 
> > similarly to the KerberizedTest category.
> > 
> > 2.) There should be well defined, thorough definitions of each test 
> > categories and these should be well documented somewhere. This is something 
> > that could be a result of a discussion amongst the contributors. After 
> > creating such definitions they could be added as javadocs to the test 
> > category interfaces as well as included in the Sqoop Developer’s Guide 
> > (changes would be needed under sqoop/src/docs/dev).
> > 
> > 3.) It would be also very useful to have exact commands described to run 
> > specific categories in the COMPILING.txt.
> > 
> > Thank you very much,
> > Bogi
> 
> Nguyen Truong wrote:
> Hi Boglarka,
> 
> Thanks for your review. I will go on and add categories for the new tests.
> 
> I don't know about the S3 tests. Could you please let me know more why 
> they should be under a separate category as KerberizedTest? Thank you very 
> much.
> 
> I will update the COMPILING.txt after we come up with the final decision 
> on the categories.
> 
> Thanks,
> Nguyen

S3 tests should be in a separate test category as they test the integration 
with the Amazon S3 cloud service that is a third party side just like an RDBMS. 
These tests also require AWS credentials to access S3 and they run only if 
these credentials are provided via the 
-Ds3.generator.command= property as well as the 
target S3 location via the -Ds3.bucket.url= property.
The S3 tests that should be in a separate category are under 
sqoop/src/test/org/apache/sqoop/s3/
The S3 related test case added in SQOOP-3368 is a simple unit test case though, 
it doesn't test the integration with S3 just the sqoop option validation 
mechanism.

I will also think about the definitions of the test categories and will share 
my thoughts later.


> On Aug. 31, 2018, 1:24 p.m., Boglarka Egyed wrote:
> > build.gradle
> > Lines 183 (patched)
> > 
> >
> > Is there any specific reason why you use "Plain" in the name of this 
> > task here? The category class being included is IntegrationTest. What's the 
> > difference?
> 
> Nguyen Truong wrote:
> They are integration tests which do not need a docker container or a 
> external database to execute. They can be run anywhere like the unit tests. 
> But they are definitely not unit tests, so I call them plain integration 
> tests. I will update the decription of it in my next patch.

I see, thanks for the explanation!


- Boglarka


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208172
---


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Aug. 28, 2018, 3:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: 

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Nguyen Truong]

> On Aug. 29, 2018, 3:39 p.m., Szabolcs Vasas wrote:
> > Hi Nguyen,
> > 
> > Thanks for submitting this, it is a great improvement!
> > 
> > Since this is a big patch we will need some more thorough review, these are 
> > my suggestions after the first iteration:
> > - org.apache.sqoop.importjob.avro.AvroImportForNumericTypesTest should be a 
> > ThirdPartyTest since it runs the numeric test on various different 
> > databases.
> > - Unfortunately there is an issue in JUnit which will prevent Gradle from 
> > executing categorized tests which are parameterized too (annotated with 
> > @RunWith). The issue is already 
> > reported(https://github.com/junit-team/junit4/issues/751) and fixed as 
> > well, but JUnit 4.13 which will contain it is not yet released... However I 
> > came up with a workaround which basically uses the JUnit upstream solution 
> > I think you should incorporate this or a similar solution in your patch: 
> > https://github.com/szvasas/sqoop/commit/6b770e2a6f0dbcef21ee063a15c2c0e8b3ab5dae

Hi Szabolcs,

Thanks a lot for your JUnit fix, I will apply your fix to the my next updated 
diff.

Best,
Nguyen


- Nguyen


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208094
---


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Aug. 28, 2018, 3:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java 

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Nguyen Truong]

> On Aug. 30, 2018, 12:53 p.m., Szabolcs Vasas wrote:
> > Hi Nguyen,
> > 
> > I found a couple of test classes which are not categorized, can you please 
> > check them?
> > 
> > org.apache.sqoop.util.TestDirCleanupHook
> > org.apache.sqoop.util.TestFileSystemUtil
> > org.apache.sqoop.util.TestMainframeFTPClientUtils
> > org.apache.sqoop.util.TestOptionsFileExpansion
> > org.apache.sqoop.util.TestSqoopJsonUtil
> > org.apache.sqoop.util.TestSubstitutionUtils

My bad, I thought they were util files so I did not check them. Will update. 
Thanks!


- Nguyen


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208128
---


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Aug. 28, 2018, 3:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 
> 276e9eaa4 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
>   src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
> f1768ee76 
>   src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
>   src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
>   src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
>   src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java 

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Nguyen Truong]

> On Aug. 29, 2018, 3:39 p.m., Szabolcs Vasas wrote:
> > build.gradle
> > Lines 216 (patched)
> > 
> >
> > I think this should depend on kerberizedTest task too so we cover all 
> > the tests which can be executed without docker containers.

Hi Szabolcs,

If we include kerberizedTest in this, do we need to use forkEvery=1 here? If 
then, it conflicts with our purpose of saving time by running with forkEvery=0.

Best,
Natalie


- Nguyen


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208094
---


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Aug. 28, 2018, 3:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 
> 276e9eaa4 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
>   src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
> f1768ee76 
>   src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
>   src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
>   src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
>   src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
>   

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Nguyen Truong]

> On Aug. 31, 2018, 1:24 p.m., Boglarka Egyed wrote:
> > Hi Nguyen,
> > 
> > Many thanks for taking care of this huge effort! Having well categorized 
> > tests is a long-awaited improvement Sqoop, it will ease the life of every 
> > developer.
> > 
> > In general your change looks good to me however I have three findings:
> > 
> > 1.) New test classes have been introduced recently in SQOOP-3348, 
> > SQOOP-3363 and SQOOP-3368 testing the Sqoop-S3 integration. I think it 
> > would make sense to have a new test category for the S3 related tests 
> > similarly to the KerberizedTest category.
> > 
> > 2.) There should be well defined, thorough definitions of each test 
> > categories and these should be well documented somewhere. This is something 
> > that could be a result of a discussion amongst the contributors. After 
> > creating such definitions they could be added as javadocs to the test 
> > category interfaces as well as included in the Sqoop Developer’s Guide 
> > (changes would be needed under sqoop/src/docs/dev).
> > 
> > 3.) It would be also very useful to have exact commands described to run 
> > specific categories in the COMPILING.txt.
> > 
> > Thank you very much,
> > Bogi

Hi Boglarka,

Thanks for your review. I will go on and add categories for the new tests.

I don't know about the S3 tests. Could you please let me know more why they 
should be under a separate category as KerberizedTest? Thank you very much.

I will update the COMPILING.txt after we come up with the final decision on the 
categories.

Thanks,
Nguyen


> On Aug. 31, 2018, 1:24 p.m., Boglarka Egyed wrote:
> > build.gradle
> > Lines 183 (patched)
> > 
> >
> > Is there any specific reason why you use "Plain" in the name of this 
> > task here? The category class being included is IntegrationTest. What's the 
> > difference?

They are integration tests which do not need a docker container or a external 
database to execute. They can be run anywhere like the unit tests. But they are 
definitely not unit tests, so I call them plain integration tests. I will 
update the decription of it in my next patch.


- Nguyen


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208172
---


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Aug. 28, 2018, 3:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Boglarka Egyed]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208172
---



Hi Nguyen,

Many thanks for taking care of this huge effort! Having well categorized tests 
is a long-awaited improvement Sqoop, it will ease the life of every developer.

In general your change looks good to me however I have three findings:

1.) New test classes have been introduced recently in SQOOP-3348, SQOOP-3363 
and SQOOP-3368 testing the Sqoop-S3 integration. I think it would make sense to 
have a new test category for the S3 related tests similarly to the 
KerberizedTest category.

2.) There should be well defined, thorough definitions of each test categories 
and these should be well documented somewhere. This is something that could be 
a result of a discussion amongst the contributors. After creating such 
definitions they could be added as javadocs to the test category interfaces as 
well as included in the Sqoop Developer’s Guide (changes would be needed under 
sqoop/src/docs/dev).

3.) It would be also very useful to have exact commands described to run 
specific categories in the COMPILING.txt.

Thank you very much,
Bogi


build.gradle
Lines 183 (patched)


Is there any specific reason why you use "Plain" in the name of this task 
here? The category class being included is IntegrationTest. What's the 
difference?


- Boglarka Egyed


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Aug. 28, 2018, 3:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Szabolcs Vasas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208128
---



Hi Nguyen,

I found a couple of test classes which are not categorized, can you please 
check them?

org.apache.sqoop.util.TestDirCleanupHook
org.apache.sqoop.util.TestFileSystemUtil
org.apache.sqoop.util.TestMainframeFTPClientUtils
org.apache.sqoop.util.TestOptionsFileExpansion
org.apache.sqoop.util.TestSqoopJsonUtil
org.apache.sqoop.util.TestSubstitutionUtils

- Szabolcs Vasas


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Aug. 28, 2018, 3:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 
> 276e9eaa4 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
>   src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
> f1768ee76 
>   src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
>   src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
>   src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
>   src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
>   src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
>   src/test/org/apache/sqoop/lib/TestBooleanParser.java 914ab37e4 
>   

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Szabolcs Vasas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208124
---



Just an FYI: I have now found another test interdependency when running the 
tests with this patch applied, submitted a fix for it in this patch: 
https://issues.apache.org/jira/browse/SQOOP-3375

- Szabolcs Vasas


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Aug. 28, 2018, 3:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 
> 276e9eaa4 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
>   src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
> f1768ee76 
>   src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
>   src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
>   src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
>   src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
>   src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
>   src/test/org/apache/sqoop/lib/TestBooleanParser.java 914ab37e4 
>   src/test/org/apache/sqoop/lib/TestClobRef.java f94d1a8af 
>   src/test/org/apache/sqoop/lib/TestFieldFormatter.java 9ac55e703 
>   src/test/org/apache/sqoop/lib/TestLargeObjectLoader.java 1e07d7174 
>   

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Szabolcs Vasas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208094
---



Hi Nguyen,

Thanks for submitting this, it is a great improvement!

Since this is a big patch we will need some more thorough review, these are my 
suggestions after the first iteration:
- org.apache.sqoop.importjob.avro.AvroImportForNumericTypesTest should be a 
ThirdPartyTest since it runs the numeric test on various different databases.
- Unfortunately there is an issue in JUnit which will prevent Gradle from 
executing categorized tests which are parameterized too (annotated with 
@RunWith). The issue is already 
reported(https://github.com/junit-team/junit4/issues/751) and fixed as well, 
but JUnit 4.13 which will contain it is not yet released... However I came up 
with a workaround which basically uses the JUnit upstream solution I think you 
should incorporate this or a similar solution in your patch: 
https://github.com/szvasas/sqoop/commit/6b770e2a6f0dbcef21ee063a15c2c0e8b3ab5dae


build.gradle
Line 139 (original), 139 (patched)


I think this task should rather depend on test and thirdPartyTest tasks 
because this would execute the third party tests without the extra properties 
defined in the thirdPartyTest task.



build.gradle
Lines 216 (patched)


I think this should depend on kerberizedTest task too so we cover all the 
tests which can be executed without docker containers.



build.gradle
Line 187 (original), 247 (patched)


This line should be rather removed because forkEvery 0 is the default 
anyway and if we specify it here then it will override all the forkEvery values 
in other test tasks so it will set the forkEvery parameter to 0 even in 
kerberizedTest task.


- Szabolcs Vasas


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Aug. 28, 2018, 3:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> 

Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[daniel voros]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/#review208088
---


Ship it!




Great stuff! Checked `test`, `unitTest` and `integrationPlainTest`.

My only concern is forgetting to apply `@Category` on future test classes. We 
wouldn't execute without that, right? Any ideas how to prevent this from 
happening?

- daniel voros


On Aug. 28, 2018, 3:52 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68541/
> ---
> 
> (Updated Aug. 28, 2018, 3:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3104
> https://issues.apache.org/jira/browse/SQOOP-3104
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We are currently unsing test naming conventions to differentiate between 
> ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
> junit categories which will allow us to have more categories in the future. 
> This would also remove the reliance on the test class name.
> 
> Test categories skeleton:
>   SqoopTest _ UnitTest
>   |__ IntegrationTest
>   |__ ManualTest
> 
>   ThirdPartyTest _ CubridTest
>|__ Db2Test
>|__ MainFrameTest
>|__ MysqlTest
>|__ NetezzaTest
>|__ OracleTest
>|__ PostgresqlTest
>|__ SqlServerTest
> 
>   KerberizedTest
> 
> Categories explanation:
> * SqoopTest: Group of the big categories, including:
> - UnitTest: It tests one class only with its dependencies mocked or 
> if the dependency
> is lightweight we can keep it. It must not start a minicluster or an 
> hsqldb database.
> It does not need JCDB drivers.
> - IntegrationTest: It usually tests a whole scenario. It may start up 
> miniclusters,
> hsqldb and connect to external resources like RDBMSs.
> - ManualTest: This should be a deprecated category which should not 
> be used in the future.
> It only exists to mark the currently existing manual tests.
> * ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC 
> driver and/or a docker
> container/external RDBMS instance to run. Subcategories express what kind 
> of external
> resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
> driver on the classpath
> * KerberizedTest: Test that needs Kerberos, which needs to be run on a 
> separate JVM.
> 
> Opinions are very welcomed. Thanks!
> 
> 
> Diffs
> -
> 
>   build.gradle fc7fc0c4c 
>   src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
>   src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
>   src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
> f5700ce65 
>   src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
> 244831672 
>   
> src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
>  d3e3fb23e 
>   src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
> 3bfb39178 
>   src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  4d2cb2f88 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
>   src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 
> 276e9eaa4 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
>   src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
> f1768ee76 
>   src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
>   src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
>   src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
>   src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
>   src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
>   src/test/org/apache/sqoop/lib/TestBooleanParser.java 914ab37e4 
>   src/test/org/apache/sqoop/lib/TestClobRef.java f94d1a8af 
>   src/test/org/apache/sqoop/lib/TestFieldFormatter.java 9ac55e703 
>   

Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

[Nguyen Truong]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68541/
---

Review request for Sqoop.


Bugs: SQOOP-3104
https://issues.apache.org/jira/browse/SQOOP-3104


Repository: sqoop-trunk


Description
---

We are currently unsing test naming conventions to differentiate between 
ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
junit categories which will allow us to have more categories in the future. 
This would also remove the reliance on the test class name.

Test categories skeleton:
  SqoopTest _ UnitTest
  |__ IntegrationTest
  |__ ManualTest

  ThirdPartyTest _ CubridTest
   |__ Db2Test
   |__ MainFrameTest
   |__ MysqlTest
   |__ NetezzaTest
   |__ OracleTest
   |__ PostgresqlTest
   |__ SqlServerTest

  KerberizedTest

Categories explanation:
* SqoopTest: Group of the big categories, including:
- UnitTest: It tests one class only with its dependencies mocked or if 
the dependency
is lightweight we can keep it. It must not start a minicluster or an 
hsqldb database.
It does not need JCDB drivers.
- IntegrationTest: It usually tests a whole scenario. It may start up 
miniclusters,
hsqldb and connect to external resources like RDBMSs.
- ManualTest: This should be a deprecated category which should not be 
used in the future.
It only exists to mark the currently existing manual tests.
* ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC driver 
and/or a docker
container/external RDBMS instance to run. Subcategories express what kind 
of external
resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
driver on the classpath
* KerberizedTest: Test that needs Kerberos, which needs to be run on a 
separate JVM.

Opinions are very welcomed. Thanks!


Diffs
-

  build.gradle fc7fc0c4c 
  src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
  src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
  src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
  src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
  src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
f5700ce65 
  src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
244831672 
  
src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
 d3e3fb23e 
  src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
3bfb39178 
  src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
  src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
  
src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java 
4d2cb2f88 
  src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
  src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
  src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
  src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
  src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 276e9eaa4 
  src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
  src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
f1768ee76 
  src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
  src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
  src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
  src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
  src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
  src/test/org/apache/sqoop/lib/TestBooleanParser.java 914ab37e4 
  src/test/org/apache/sqoop/lib/TestClobRef.java f94d1a8af 
  src/test/org/apache/sqoop/lib/TestFieldFormatter.java 9ac55e703 
  src/test/org/apache/sqoop/lib/TestLargeObjectLoader.java 1e07d7174 
  src/test/org/apache/sqoop/lib/TestRecordParser.java d6844c1cf 
  src/test/org/apache/sqoop/manager/TestDefaultManagerFactory.java 8e1632430 
  src/test/org/apache/sqoop/manager/TestMainframeManager.java c84f05f66 
  src/test/org/apache/sqoop/manager/TestSqlManager.java 185f5a7a1 
  src/test/org/apache/sqoop/manager/cubrid/CubridAuthTest.java 82fac12e3 
  src/test/org/apache/sqoop/manager/cubrid/CubridCompatTest.java 8a075e87d 
  src/test/org/apache/sqoop/manager/cubrid/CubridManagerExportTest.java 
4de8e40fd 
  src/test/org/apache/sqoop/manager/cubrid/CubridManagerImportTest.java 
addf1aeec 
  
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManualTest.java
 d1a6d6926