sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 41910: SQOOP-2758: Sqoop2: Add integration test for shell
Date Thu, 07 Jan 2016 19:13:03 GMT


> On Jan. 6, 2016, 3:57 p.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/ShellTestCase.java, lines 47-49
> > <https://reviews.apache.org/r/41910/diff/1-2/?file=1181528#file1181528line47>
> >
> >     This seems quite hacky to me because nobody will change the SQOOP_CLUSTER_CLASS
back to it's original value when this test case will be done. I think that we have to introduce
better way how to override the default class if needed. 
> >     
> >     The need to specify different minicluster will be required for more then just
this testcase (Derby*Upgrade or Classpath related tests came to mind). Hence having proper
way how to handle that will be required. I believe that Dian is working on those at the moment,
so perhaps he might have an idea already?
> 
> Colin Ma wrote:
>     Thanks for catch this problem, update the patch and the solution as following:
>     1. When init the ShellTestCase, backup the property "sqoop.minicluster.class" and
set the JettySqoopMiniClusterWithExternalConnector as a new value. 
>     2. Add @AfterSuite in ShellTestCase, to restore the property "sqoop.minicluster.class"
with the old value.
>     The property "sqoop.minicluster.class" change will be only for shell tests, you can
refer the patch for detail, feel free for any comments.

I'm starting to think that we might want to take this particular discussion to a separate
JIRA that is subtask of SQOOP-2329.

Thinking about this one a bit more, I don't think that it will really work for all cases (e.g.
for Derby*Ugprade, Classpath related and such ...). The way we are loading infrastructure
providers is once for the whole suite and not for each test or test class (unless I've missed
something somewhere).  So perhaps we should kick the tests needing different provider to a
different suite. I know that you have it already here in this patch for shell, but I'm thinking
for all the other tests that will need the same.

Also this way seems really, really hacky to me. Perhaps we can do it a bit differently and
provide parameters to the annotations? Similarly as we have for validators here [1]. So that
it would like this:

@Infrastructure({@Infra(KdcInfraProvider.class}, @Infra(HadoopInfraProvider.class), @Infra(SqoopInfraProvider.class,
arg = "DifferentMiniClusterClass")}) ...

Those points seems irrelevant to this change, hence my suggestion to open a new JIRA and solve
that problem first.

What do you think?

Links:
1: https://github.com/apache/sqoop/blob/sqoop2/common/src/main/java/org/apache/sqoop/model/Validator.java


- Jarek


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41910/#review113082
-----------------------------------------------------------


On Jan. 7, 2016, 3:43 a.m., Colin Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41910/
> -----------------------------------------------------------
> 
> (Updated Jan. 7, 2016, 3:43 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> For the test of shell, currently, too many mock in test cases, and some bugs won't be
detected. The integration test should be added for shell, and do the test with sqoop server.
> 
> 
> Diffs
> -----
> 
>   shell/src/main/java/org/apache/sqoop/shell/SetCommand.java 0a04e3d 
>   shell/src/main/java/org/apache/sqoop/shell/ShowCommand.java c148eeb 
>   shell/src/main/java/org/apache/sqoop/shell/StartCommand.java 679c1f7 
>   shell/src/main/java/org/apache/sqoop/shell/StatusCommand.java 6082799 
>   shell/src/main/java/org/apache/sqoop/shell/StopCommand.java 83c571a 
>   test/pom.xml bd1680f 
>   test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java becfa6b

>   test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniClusterWithExternalConnector.java
PRE-CREATION 
>   test/src/main/java/org/apache/sqoop/test/testcases/ConnectorClasspathTestCase.java
6db1db8 
>   test/src/main/java/org/apache/sqoop/test/testcases/ShellTestCase.java PRE-CREATION

>   test/src/main/java/org/apache/sqoop/test/utils/ConnectorUtils.java PRE-CREATION 
>   test/src/test/java/org/apache/sqoop/integration/connectorloading/ClasspathTest.java
4bb6aa1 
>   test/src/test/java/org/apache/sqoop/integration/connectorloading/ConnectorClasspathIsolationTest.java
5b95631 
>   test/src/test/java/org/apache/sqoop/integration/server/SubmissionWithDisabledModelObjectsTest.java
9adebea 
>   test/src/test/java/org/apache/sqoop/integration/shell/CloneCommandTest.java PRE-CREATION

>   test/src/test/java/org/apache/sqoop/integration/shell/CreateCommandTest.java PRE-CREATION

>   test/src/test/java/org/apache/sqoop/integration/shell/DeleteCommandTest.java PRE-CREATION

>   test/src/test/java/org/apache/sqoop/integration/shell/DisableCommandTest.java PRE-CREATION

>   test/src/test/java/org/apache/sqoop/integration/shell/EnableCommandTest.java PRE-CREATION

>   test/src/test/java/org/apache/sqoop/integration/shell/SetCommandTest.java PRE-CREATION

>   test/src/test/java/org/apache/sqoop/integration/shell/ShowCommandTest.java PRE-CREATION

>   test/src/test/java/org/apache/sqoop/integration/shell/StartCommandTest.java PRE-CREATION

>   test/src/test/java/org/apache/sqoop/integration/shell/StatusCommandTest.java PRE-CREATION

>   test/src/test/java/org/apache/sqoop/integration/shell/StopCommandTest.java PRE-CREATION

>   test/src/test/java/org/apache/sqoop/integration/shell/UpdateCommandTest.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestConnectorForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestExtractorForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestFromDestroyerForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestFromInitializerForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestFromJobConfigForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestFromJobConfigurationForShell.java
PRE-CREATION 
>   test/src/test/resources/TestConnectorForShell/TestLinkConfigForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestLinkConfigurationForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestLoaderForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestPartitionForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestPartitionerForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestToDestroyerForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestToInitializerForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestToJobConfigForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/TestToJobConfigurationForShell.java PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/sqoopconnector.properties PRE-CREATION

>   test/src/test/resources/TestConnectorForShell/test-connector-for-shell.properties PRE-CREATION

>   test/src/test/resources/shell-tests-suite.xml PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41910/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Colin Ma
> 
>


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