> On April 7, 2017, 8:11 a.m., Attila Szabo wrote: > > Hey Bogi, > > > > I've got two concerns related to this change: > > - The change set is quite huge, and thus not easy to review. Could we > > please split up to smaller pieces? > > - Besides the fact that this modification will provide some way to execute > > these tests through Junit and some ant tasks, I can't really spot how this > > change will make the test cases more "automated". Will these tests be > > included in the CI cycle? Do we already have a SqlServer instance connected > > to the Apache CI system we can test against? Which CI cycle would include > > the execution of this test suite? > > > > Thanks for your answers and clarification, > > > > Attila
Hi Attila, Thanks for your inputs. However it is a bigger change set, I would not split it into smaller pieces as I think this is a coherent whole representing one logical change. Every file contains similar, limited amount of modifications which makes easier to review it IMHO. Automation means several things here. First, these all were manual tests meaning these were executed possibly never or a really long time ago as there were numerous tests failing. Also, these were able to be executed by some manual ant task which was quite difficult because of the hard coded DB related variables and thus were possibly avoided instead. But from now they can be executed as part of the 3rd party test suite by setting the DB connect string and credentials through system properties. This definitely makes easier to test changes which possibly affect integration with DBs as we already have the same practice for MySQL, Postgre, etc. Unfortunately, even the 3rd party test suite is not a part of CI cycle, however, it totally makes sense to add it too and it should be. Fortunately, AFAIR there are plans to improve Sqoop CI cycles, the related communication has already started at dev@ and is an ongoing effort thus I wouldn't think that it should be part of the scope of this change - this change is a good first step toward it, however. Many thanks, Bogi - Boglarka ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58233/#review171319 ----------------------------------------------------------- On April 6, 2017, 5:27 p.m., Boglarka Egyed wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58233/ > ----------------------------------------------------------- > > (Updated April 6, 2017, 5:27 p.m.) > > > Review request for Sqoop, Anna Szonyi, Szabolcs Vasas, and Liz Szilagyi. > > > Bugs: SQOOP-3167 > https://issues.apache.org/jira/browse/SQOOP-3167 > > > Repository: sqoop-trunk > > > Description > ------- > > Automated SQLServer Manual tests including test case correction and minor > rework too: > - modified connection string setup to make them able to run automatically > - fixed failing test cases > - excluded invalid test cases > - added database cleanup logic in tearDown part > - updated java docs > - removed unused imports > - changing names to add them to the 3rd party test suite > > Note: A more extensive refactor of the test classes in > org.apache.sqoop.manager.sqlserver could be made, it will be addressed in > another JIRA as that is a different scope. > > > Diffs > ----- > > build.xml 73db28b272c50b4f76fef8421e6b9dfe5fed40f4 > src/test/com/cloudera/sqoop/hive/TestHiveImport.java > 33e0cc41f6f379bac2085431e0f1adc60bce6bce > src/test/com/cloudera/sqoop/manager/SQLServerManagerExportManualTest.java > 9a92479245fa35c210d8e49f847292ee53d6f9b1 > src/test/com/cloudera/sqoop/manager/SQLServerManagerImportManualTest.java > 1f69725da8408853ac55b1f316ce1b9ef015e674 > src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java > 851bf49614e829d07de252b83f4ad550d0cb043b > src/test/org/apache/sqoop/manager/sqlserver/ManagerCompatExport.java > 8c5176ad61aae61b96c7458d3b4b83dc11960268 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeExportDelimitedFileManualTest.java > 099d7344beb428c58b32d926af5ea079211da490 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeExportSequenceFileManualTest.java > 21676f02510693dcdd856a1d9dfba7d05eace023 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportDelimitedFileManualTest.java > 519fb525bdbb167520368d404667036669925041 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportSequenceFileManualTest.java > a0dad8a60b99d522ad3691e15b8b16c56e4b5858 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerHiveImportManualTest.java > 1999272181421a539318ed195ea4257f52b2ed08 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerManualTest.java > 1178e3c79de4d0b5c7a96c6ad7eb316ed15e47c4 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerMultiColsManualTest.java > 6a8ab51967237f471044b868615fdb3e057b1d92 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerMultiMapsManualTest.java > c9a5b5ef596cfc1b28948c2a071935dfb9500cde > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerParseMethodsManualTest.java > cd05aecf1ae5bd79fb485325d58b33a73e9df290 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerQueryManualTest.java > 0057ac9df562c8e92cf7b9014c5e4239886a8104 > src/test/org/apache/sqoop/manager/sqlserver/SQLServerSplitByManualTest.java > f85245ab8cdd66da983ac9017d356f251f22e7db > src/test/org/apache/sqoop/manager/sqlserver/SQLServerWhereManualTest.java > 10ae03b324b15f5ea0cc3cbbc04d3a5041233dd9 > > > Diff: https://reviews.apache.org/r/58233/diff/2/ > > > Testing > ------- > > ant clean test, ant test > > ant clean test -Dthirdparty=true -Dsqoop.thirdparty.lib.dir=3rdpartylib > -Dsqoop.test.sqlserver.connectstring.host_url=sqlserver > -Dsqoop.test.sqlserver.database=databasename -Dms.sqlserver.username=username > -Dms.sqlserver.password=password -Dtestcase=SQLServer* > > ant clean test -Dthirdparty=true -Dsqoop.thirdparty.lib.dir=3rdpartylib > -Dsqoop.test.mysql.connectstring.host_url=mysql > -Dsqoop.test.mysql.databasename=databasename > -Dsqoop.test.mysql.password=password -Dsqoop.test.mysql.username=username > -Dsqoop.test.oracle.connectstring=oracle > -Dsqoop.test.postgresql.connectstring.host_url=postgre > -Dsqoop.test.cubrid.connectstring.host_url=cubrid > -Dsqoop.test.cubrid.connectstring.username=username > -Dsqoop.test.cubrid.connectstring.database=database > -Dsqoop.test.cubrid.connectstring.password=password > -Dmapred.child.java.opts="-Djava.security.egd=file:/dev/../dev/urandom" > -Dtest.timeout=1000000 > -Dsqoop.test.sqlserver.connectstring.host_url=sqlserver > -Dsqoop.test.sqlserver.database=database -Dms.sqlserver.username=username > -Dms.sqlserver.password=password > > > Thanks, > > Boglarka Egyed > >
