----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67429/#review204767 -----------------------------------------------------------
Hi Chris! Thank you for your patch this is a great addition to our mainframe connector testing! Could we use this image for the other mainframe tests as well? When I tried to run it on my machine I got an exception first the reason was that org.apache.sqoop.manager.MainframeManager#options had to be deleted because it is already added to ConnManager, can you please include this change in your patch as well? See my other comments inline. build.xml Lines 887 (patched) <https://reviews.apache.org/r/67429/#comment287442> Do we need to be able to set gdg, gdg.filename and gdg.md5 from Ant? For host, port, username and password it makes sense because in theory they could be different in some test environments but gdg, gdg.filename and gdg.md5 seems to be a test data which could be defined as a static field in your test only. src/scripts/thirdpartytest/docker-compose/sqoop-thirdpartytest-db-services.yml Lines 115 (patched) <https://reviews.apache.org/r/67429/#comment287467> Did you create this image using a dockerfile? If yes, is it possible to attach to include it in your patch? src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java Lines 42 (patched) <https://reviews.apache.org/r/67429/#comment287443> There are unfortunately too many StringUtils classes on the classpath, can you please use org.apache.commons.lang3.StringUtils? src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java Lines 112 (patched) <https://reviews.apache.org/r/67429/#comment287464> Do we need this method here? It initializes a MainframeManager but it is not used later and I can see that similar parameters are configured in getArgv method. src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java Lines 163 (patched) <https://reviews.apache.org/r/67429/#comment287444> Do we need to redefine runImport here or can we just use org.apache.sqoop.testutil.ImportJobTestCase#runImport(org.apache.sqoop.tool.SqoopTool, java.lang.String[])? src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java Lines 185 (patched) <https://reviews.apache.org/r/67429/#comment287445> Can we remove this comment? src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java Lines 216 (patched) <https://reviews.apache.org/r/67429/#comment287463> You can remove this method since it is empty. - Szabolcs Vasas On June 5, 2018, 1:23 p.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67429/ > ----------------------------------------------------------- > > (Updated June 5, 2018, 1:23 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-trunk > > > Description > ------- > > Added simple test for GDG dataset. This depends on a bug being fixed when > SQOOP-3224 ships. > > > Diffs > ----- > > build.xml a85705fa > > src/scripts/thirdpartytest/docker-compose/sqoop-thirdpartytest-db-services.yml > 2f4a07f6 > src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/67429/diff/3/ > > > Testing > ------- > > Running the test on a patched version of trunk. > > > Thanks, > > Chris Teoh > >
