----------------------------------------------------------- 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) <https://reviews.apache.org/r/68541/#comment292409> 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) <https://reviews.apache.org/r/68541/#comment292413> 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) <https://reviews.apache.org/r/68541/#comment292410> This class should be an IntegrationTest too. src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java Lines 80 (patched) <https://reviews.apache.org/r/68541/#comment292411> 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) <https://reviews.apache.org/r/68541/#comment292412> This test should be an integrationTest too. src/test/org/apache/sqoop/util/BlockJUnit4ClassRunnerWithParameters.java Lines 1 (patched) <https://reviews.apache.org/r/68541/#comment292406> Please add license header to this new file. src/test/org/apache/sqoop/util/BlockJUnit4ClassRunnerWithParametersFactory.java Lines 1 (patched) <https://reviews.apache.org/r/68541/#comment292407> Please add license header to this new file. src/test/org/apache/sqoop/util/BlockJUnit4ClassRunnerWithParametersFactory.java Lines 8 (patched) <https://reviews.apache.org/r/68541/#comment292408> 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 > |__ 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 > 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 > src/test/org/apache/sqoop/manager/db2/DB2ManagerImportManualTest.java > b5d47f2ed > src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java > 393a110fb > src/test/org/apache/sqoop/manager/hsqldb/TestHsqldbManager.java 745a8125d > src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java > 3b8ed2361 > src/test/org/apache/sqoop/manager/mysql/DirectMySQLExportTest.java > b3570ff1f > src/test/org/apache/sqoop/manager/mysql/DirectMySQLTest.java 89a7fec6e > src/test/org/apache/sqoop/manager/mysql/JdbcMySQLExportTest.java f655bcc8a > src/test/org/apache/sqoop/manager/mysql/MySQLAllTablesTest.java baf0e2a71 > src/test/org/apache/sqoop/manager/mysql/MySQLAuthTest.java 1e2f70d23 > src/test/org/apache/sqoop/manager/mysql/MySQLCompatTest.java 7e822e66f > src/test/org/apache/sqoop/manager/mysql/MySQLFreeFormQueryTest.java > f4f0b7415 > src/test/org/apache/sqoop/manager/mysql/MySQLLobAvroImportTest.java > 6208975fc > src/test/org/apache/sqoop/manager/mysql/MySqlCallExportTest.java 22a66761c > src/test/org/apache/sqoop/manager/mysql/MySqlColumnEscapeImportTest.java > eaab8c578 > > src/test/org/apache/sqoop/manager/netezza/DirectNetezzaExportManualTest.java > 0a6997fa3 > > src/test/org/apache/sqoop/manager/netezza/DirectNetezzaHCatExportManualTest.java > 9365ba0f3 > > src/test/org/apache/sqoop/manager/netezza/DirectNetezzaHCatImportManualTest.java > c05b73332 > src/test/org/apache/sqoop/manager/netezza/NetezzaExportManualTest.java > 95abe7a6e > src/test/org/apache/sqoop/manager/netezza/NetezzaImportManualTest.java > 4002c647a > > src/test/org/apache/sqoop/manager/oracle/OraOopDataDrivenDBInputFormatConnectionCloseTest.java > bb33c3547 > src/test/org/apache/sqoop/manager/oracle/OraOopTestCase.java 1bae71cb0 > src/test/org/apache/sqoop/manager/oracle/OracleCallExportTest.java > 6d6602a7b > src/test/org/apache/sqoop/manager/oracle/OracleColumnEscapeImportTest.java > 684586c8d > src/test/org/apache/sqoop/manager/oracle/OracleCompatTest.java 553096a87 > src/test/org/apache/sqoop/manager/oracle/OracleExportTest.java a880af36d > src/test/org/apache/sqoop/manager/oracle/OracleFreeFormQueryTest.java > bb3e7c460 > src/test/org/apache/sqoop/manager/oracle/OracleIncrementalImportTest.java > 8e6ccc96a > src/test/org/apache/sqoop/manager/oracle/OracleLobAvroImportTest.java > 525ccf457 > src/test/org/apache/sqoop/manager/oracle/OracleManagerTest.java 9251f0266 > > src/test/org/apache/sqoop/manager/oracle/OracleSpecialCharacterTableImportTest.java > 6539e5a9b > src/test/org/apache/sqoop/manager/oracle/OracleSplitterTest.java c2f9532c1 > > src/test/org/apache/sqoop/manager/postgresql/DirectPostgreSQLExportManualTest.java > 22b202a20 > > src/test/org/apache/sqoop/manager/postgresql/PGBulkloadManagerManualTest.java > 8855316c8 > src/test/org/apache/sqoop/manager/postgresql/PostgresqlExportTest.java > f86b1192c > > src/test/org/apache/sqoop/manager/postgresql/PostgresqlExternalTableImportTest.java > dd4cfb48b > src/test/org/apache/sqoop/manager/postgresql/PostgresqlImportTest.java > b8aa17b03 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeExportDelimitedFileTest.java > 9b70af181 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeExportSequenceFileTest.java > 293da0002 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportDelimitedFileTest.java > 520c4ac8b > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportSequenceFileTest.java > 592a78f22 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerHiveImportTest.java > e6b086550 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerExportTest.java > b7c2b75d6 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java > 79e37f08f > src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerTest.java > fdf856be1 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerMultiColsTest.java > fb765fb83 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerMultiMapsTest.java > 5e89cc9a9 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerParseMethodsTest.java > fbd8d9633 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerQueryTest.java > e0c8d6724 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerSplitByTest.java > a1c220192 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerWhereTest.java > 11d0963f2 > > src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java > c0d0a248f > src/test/org/apache/sqoop/mapreduce/TestJdbcExportJob.java 81ab6772d > src/test/org/apache/sqoop/mapreduce/TestJobBase.java e1781bb63 > src/test/org/apache/sqoop/mapreduce/db/TestBigDecimalSplitter.java > 951a3dc55 > src/test/org/apache/sqoop/mapreduce/db/TestDBConfiguration.java 3160db956 > src/test/org/apache/sqoop/mapreduce/db/TestDataDrivenDBInputFormat.java > 9e538fd91 > src/test/org/apache/sqoop/mapreduce/db/TestIntegerSplitter.java b43fc41ff > src/test/org/apache/sqoop/mapreduce/db/TestSQLServerDBRecordReader.java > 70187b17e > src/test/org/apache/sqoop/mapreduce/db/TestTextSplitter.java 5d9cdf05b > > src/test/org/apache/sqoop/mapreduce/db/TextSplitterHadoopConfIntegrationTest.java > 9eb8922d5 > src/test/org/apache/sqoop/mapreduce/hcat/TestSqoopHCatImportHelper.java > 3f734ea34 > src/test/org/apache/sqoop/mapreduce/hcat/TestSqoopHCatUtilities.java > dff11f1e5 > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java > 3547294fc > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetInputFormat.java > efef056bc > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetInputSplit.java > 5d92f6d51 > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetPath.java > 9b277b2ac > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileEntryParser.java > eb0f8c009 > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeImportJob.java > be62efd04 > > src/test/org/apache/sqoop/mapreduce/sqlserver/SqlServerUpsertOutputFormatTest.java > a89e8005e > src/test/org/apache/sqoop/metastore/PasswordRedactorTest.java a2dbc7185 > src/test/org/apache/sqoop/metastore/SavedJobsTestBase.java 9c9b2f441 > src/test/org/apache/sqoop/metastore/TestAutoGenericJobStorage.java > d5424c6fc > src/test/org/apache/sqoop/metastore/TestGenericJobStorage.java 026fbdee4 > src/test/org/apache/sqoop/metastore/TestGenericJobStorageValidate.java > 9995a425e > > src/test/org/apache/sqoop/metastore/TestMetastoreConfigurationParameters.java > 5a6fac569 > src/test/org/apache/sqoop/metastore/db2/DB2JobToolTest.java b2b1fb62a > > src/test/org/apache/sqoop/metastore/db2/DB2MetaConnectIncrementalImportTest.java > e7969faaa > src/test/org/apache/sqoop/metastore/db2/DB2SavedJobsTest.java caf753c81 > src/test/org/apache/sqoop/metastore/mysql/MySqlJobToolTest.java 2ec9648fc > > src/test/org/apache/sqoop/metastore/mysql/MySqlMetaConnectIncrementalImportTest.java > e19bbc831 > src/test/org/apache/sqoop/metastore/mysql/MySqlSavedJobsTest.java e15c322d5 > src/test/org/apache/sqoop/metastore/oracle/OracleJobToolTest.java a3e61e98b > > src/test/org/apache/sqoop/metastore/oracle/OracleMetaConnectIncrementalImportTest.java > 37beaa44c > src/test/org/apache/sqoop/metastore/oracle/OracleSavedJobsTest.java > 4691530ff > src/test/org/apache/sqoop/metastore/postgres/PostgresJobToolTest.java > 065e1bbdb > > src/test/org/apache/sqoop/metastore/postgres/PostgresMetaConnectIncrementalImportTest.java > 0ffbf5ad7 > src/test/org/apache/sqoop/metastore/postgres/PostgresSavedJobsTest.java > ee3f00564 > src/test/org/apache/sqoop/metastore/sqlserver/SqlServerJobToolTest.java > 87d7b3433 > > src/test/org/apache/sqoop/metastore/sqlserver/SqlServerMetaConnectIncrementalImportTest.java > f1a2a662a > src/test/org/apache/sqoop/metastore/sqlserver/SqlServerSavedJobsTest.java > b37623b82 > src/test/org/apache/sqoop/orm/TestClassWriter.java 0cc07cf88 > src/test/org/apache/sqoop/orm/TestCompilationManager.java abd72d850 > src/test/org/apache/sqoop/testcategories/KerberizedTest.java PRE-CREATION > src/test/org/apache/sqoop/testcategories/sqooptest/IntegrationTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/sqooptest/ManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/sqooptest/SqoopTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/sqooptest/UnitTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/thirdpartytest/CubridTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/thirdpartytest/Db2Test.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/thirdpartytest/MainFrameTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/thirdpartytest/MysqlTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/thirdpartytest/NetezzaTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/thirdpartytest/OracleTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/thirdpartytest/PostgresqlTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/thirdpartytest/SqlServerTest.java > PRE-CREATION > src/test/org/apache/sqoop/testcategories/thirdpartytest/ThirdPartyTest.java > PRE-CREATION > src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java fe6ba8311 > src/test/org/apache/sqoop/testutil/TestArgumentArrayBuilder.java 6d701ab94 > src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java bdac437f1 > src/test/org/apache/sqoop/tool/TestBaseSqoopTool.java 5571b25a1 > src/test/org/apache/sqoop/tool/TestExportToolValidateOptions.java f16d1873e > src/test/org/apache/sqoop/tool/TestHiveServer2OptionValidations.java > ed4b5a498 > src/test/org/apache/sqoop/tool/TestImportTool.java 8c2be3bab > src/test/org/apache/sqoop/tool/TestToolPlugin.java 19dea221a > src/test/org/apache/sqoop/tool/TestValidateImportOptions.java 9b61bd5d9 > src/test/org/apache/sqoop/util/BlockJUnit4ClassRunnerWithParameters.java > PRE-CREATION > > src/test/org/apache/sqoop/util/BlockJUnit4ClassRunnerWithParametersFactory.java > PRE-CREATION > src/test/org/apache/sqoop/util/TestDirCleanupHook.java 41146fd17 > src/test/org/apache/sqoop/util/TestFileSystemUtil.java 28fb58ceb > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194c > src/test/org/apache/sqoop/util/TestOptionsFileExpansion.java 3fc9e6005 > src/test/org/apache/sqoop/util/TestSqoopJsonUtil.java fdf972c11 > src/test/org/apache/sqoop/util/TestSubstitutionUtils.java a2ac3410a > src/test/org/apache/sqoop/validation/AbortOnFailureHandlerTest.java > ee04563c4 > src/test/org/apache/sqoop/validation/AbsoluteValidationThresholdTest.java > 86a99c445 > > > Diff: https://reviews.apache.org/r/68541/diff/2/ > > > Testing > ------- > > Ran unit tests, integration plain tests, and third party tests. > You can run unit tests and integration plain tests together with ./gradlew > test > or separately with ./gradlew unitTest and ./gradlew integrationPlainTest > > > File Attachments > ---------------- > > SQOOP-3104.patch > > https://reviews.apache.org/media/uploaded/files/2018/08/28/7058a562-ccef-4ca3-8b58-bd6a6e6ec377__SQOOP-3104.patch > Test order in my machine > > https://reviews.apache.org/media/uploaded/files/2018/09/08/4af4030f-1230-402b-99c7-51d2d37da524__SqoopTestOrder.txt > > > Thanks, > > Nguyen Truong > >
