sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Szabolcs Vasas <vasas.szabo...@gmail.com>
Subject Re: Review Request 67429: SQOOP-3331: Add mainframe integration test for GDG
Date Thu, 14 Jun 2018 12:35:39 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message