----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41910/#review113082 -----------------------------------------------------------
Overall the new approach looks good to me, thanks for incorporating the suggestion! I have one outstanding concern: test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java (line 131) <https://reviews.apache.org/r/41910/#comment173595> I'm wondering why this change? I believe that one should have the ability to override the MINICLUSTER_CLASS_PROPERTY from command line if needed, but this will prohibit that (for example if we will need to run the integration tests on real cluster). test/src/main/java/org/apache/sqoop/test/testcases/ShellTestCase.java (lines 44 - 46) <https://reviews.apache.org/r/41910/#comment173596> 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? Jarcec - Jarek Cecho On Jan. 6, 2016, 1:51 p.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41910/ > ----------------------------------------------------------- > > (Updated Jan. 6, 2016, 1:51 p.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 > c300b33 > > 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 > >
