----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67429/#review204825 -----------------------------------------------------------
Hi Chris, I think addig this patch is a great first step to enable better testing of Mainframe use cases. I could set up the container correctly and MainframeManagerImportTest ran successfully. I had a couple of findings though, please find them below. Thanks, Bogi src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java Lines 49 (patched) <https://reviews.apache.org/r/67429/#comment287564> Could you please make this comment a proper javadov by using here /** src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java Lines 68-91 (patched) <https://reviews.apache.org/r/67429/#comment287565> Would you mind creating a separate MainframeTestUtil class to have these common code parts? I think it will be cleaner when there will be new tests added. See for example org.apache.sqoop.manager.postgresql.PostgresqlTestUtil src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java Lines 105 (patched) <https://reviews.apache.org/r/67429/#comment287568> Here could be a javadoc again explaining the parameters properly. src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java Lines 121-122 (patched) <https://reviews.apache.org/r/67429/#comment287566> Could you please use LOG.error("Got IOException during import: ", ioe); here instead of this? src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java Lines 123 (patched) <https://reviews.apache.org/r/67429/#comment287567> I think it would be more convenient to throw a RuntimeException here instead of having the test failed that would mean "test did not pass" instead of "something went wrong during test executin". See for example org.apache.sqoop.testutil.AvroTestUtils#verify - Boglarka Egyed On June 15, 2018, 7:49 a.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67429/ > ----------------------------------------------------------- > > (Updated June 15, 2018, 7:49 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-trunk > > > Description > ------- > > Added simple test for GDG dataset. Fixed bug triggered by recent Parquet > patch. > > > Diffs > ----- > > build.xml a85705fa > src/java/org/apache/sqoop/manager/MainframeManager.java 4e8be155 > > 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/5/ > > > Testing > ------- > > Running the integration and unit test on a patched version of trunk. > > > Thanks, > > Chris Teoh > >
