----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58233/#review171363 -----------------------------------------------------------
Hi Bogi, Liz, Most probably I did not phrase my thoughts precise enough in the previous round, so please let me express myself in a bit more direct and clear way: My problem is not with the size of the patch file (although IMHO ~1000+ changed lines is quite a few), but with the fact that this change tries to achieve multiple things, which I think would make more sense as part of multiple issues/commits. Here are the things I've identified as the effects/consequences of this changeset: - It delivers improvements/fixes around the way connections are handled in the MSSqlServer related tests, which is a great thing, makes the usage much more flexible. -- Although around the deletes (were those codes really dead?)/fixes I'm not sure if those things would be in sync with the design goals of the SqlServer connector, but I'm not used to consider myself as a MSSqlServer expert. - Pushes the SqlServer related manual tests into the third party ant test cycle. IMHO this is not the best decision in the current test + CI architecture, as from that moment this patch would have been committed, it would force every contributor to have a working MSSqlServer instance (on the top of the currently existing ones including MySQL, PostgreSQL, Oracle, Cubrid, etc.) on their dev infrastructure, or facing with the fact that some of the third party tests will fail continuously on their side (which does not sound like a best practice). Most probably on your side it didn't appear as a problem as you do have standalone instances for yourself, but we cannot depend on that this is true in case of every contributor. Although the test files themselves contains some very basic instructions how to make the tests work, but still in the current version it would need manual interactions+installs, thus won't work out of the box (needless to say that the instructions are very much not deta iled enough to someone who is not expert in installing MSSqlServer). -- As a subpart of this section, I have to highlight that this way of changes alternates the intention of the original devs (and I'm pretty sure they had good reason why these tests were not automated but manual). -- Introducing just another external dependency won't help to onboard new contributors (just another new -D param for a process which is not quite good documented already) -- It also won't make the CI integration easier in the future - The title+description JIRA is missleading as it does not solve the true automation (neither on CI level, nor on the side of the devs). So if you trust in my experience and wisdom as a committer you would consider splitting up this changeset into 2-3 parts, and only after pushing the tests into the 3rd party cycle, when the SqlServer deployment is solved out of the box (e.g. with docker or global apache installation or ansible or so). Please also consider fixing that part what I've identified as an issue My 2 cents, Attila src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportDelimitedFileManualTest.java Lines 224-242 (original), 249-299 (patched) <https://reviews.apache.org/r/58233/#comment244257> Hi, Could you please give me some explanation around these test cases? For me it looks strange intentionally, that we're trying to leverage from some base/super test class, but nullifying some of it's test cases with an empty method body. If these things are not neccessary I would suggest deleting them, as the current solution is very much misleading, b/c these cases won't look as 'invalid' as you marked them, but very much passed green test cases, giving the false intention these cases are very much supported in the delimited case as well. If you need a common ancestor I would advise using 'extract superclass' refactoring. - Attila Szabo 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 > >
