----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31824/#review80876 -----------------------------------------------------------
Hi Abe, thank you very much for working on this one! I have several questions/suggestions inline and a high level questions: It seems that when running integration tests, all that user sees is this output on the console: Running TestSuite Tests run: 26, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 905.703 sec I'm wondering if there is a way how to report more granual progress then "working" and "done"? Like "Executed 5 from 26 tests" or something similar? I'm a bit concerned that this is step back in usability as don't have easy way to see what are the tests doing - we would have to wait entire time for them to finish to even noticed that something is failing. common-test/src/main/java/org/apache/sqoop/common/test/kafka/KafkaRunnerFactory.java <https://reviews.apache.org/r/31824/#comment131017> This seems unrelated change to this patch? common-test/src/main/java/org/apache/sqoop/common/test/kafka/TestUtil.java <https://reviews.apache.org/r/31824/#comment131018> I'm wondering if I'm missing something or whether this just moves the code around? If so, is it really required? test/pom.xml <https://reviews.apache.org/r/31824/#comment131019> Would it make sense to drop the excludes when we're not using them anymore? test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java <https://reviews.apache.org/r/31824/#comment131020> Is this intentional change? test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java <https://reviews.apache.org/r/31824/#comment131022> I'm wondering why we are removing this one? test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java <https://reviews.apache.org/r/31824/#comment131024> I'm wondering if this change is related to the patch? I think that it's a good idea to make the topic name configurable, but that seems as orthogonal issue and hence should be perhaps solved in separate JIRA? test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java <https://reviews.apache.org/r/31824/#comment131025> If we are defining the method name in the parent class of all tests, let's drop it from the children test classes? I know that several tests are using this functionality. test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java <https://reviews.apache.org/r/31824/#comment131026> We're changing the default value here, so I'm wondering if it's intentional? test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java <https://reviews.apache.org/r/31824/#comment131028> I'm wondering what is the reasoning to drop the directory containing all the tests? That will make it quite hard to follow as target/ will be full of "random" directories right? Isn't it better to put together all the tests into the same subdirectory like target/cargo-test-output/*? test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java <https://reviews.apache.org/r/31824/#comment131029> I'm not sure that I'm following up this change correctly, are we simply removing unused variable? test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java <https://reviews.apache.org/r/31824/#comment131031> It seems that we're re-classifying this test, so I'm wondering whether this should be done in separate JIRA? test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java <https://reviews.apache.org/r/31824/#comment131030> Seems as a lot of unrelated changes and moves around? test/src/test/resources/suite.xml <https://reviews.apache.org/r/31824/#comment131035> Do you think that we will add more suits in the future? If so, it would perhaps make sense to rename the file to "IntegrationTests.xml" or something similar right? test/src/test/resources/suite.xml <https://reviews.apache.org/r/31824/#comment131032> I'm wondering if it would be feasilbe to use org.apache.integration.connector here? I think that it will be error prone to force us to create new entry for single new package that we will create. Jarcec - Jarek Cecho On April 19, 2015, 9:02 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31824/ > ----------------------------------------------------------- > > (Updated April 19, 2015, 9:02 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1952 > https://issues.apache.org/jira/browse/SQOOP-1952 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit d489e803cc01127ded5270bbe7cf640e674648aa > Author: Abraham Elmahrek <[email protected]> > Date: Fri Mar 6 17:15:06 2015 -0800 > > SQOOP-1952 > > :100755 100755 b1acdc4... 4a5ce80... M dev-support/test-patch.py > :100644 100644 32afc77... 1b30782... M pom.xml > :100644 100644 f743d25... d2761f6... M test/pom.xml > :100644 100644 ce6af6e... 90afef1... M > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > :000000 100644 0000000... 2febf5e... A test/src/test/resources/suite.xml > > > Diffs > ----- > > > common-test/src/main/java/org/apache/sqoop/common/test/kafka/KafkaRunnerFactory.java > b26ca74 > common-test/src/main/java/org/apache/sqoop/common/test/kafka/TestUtil.java > 09ddcc7 > pom.xml c608ca7 > test/pom.xml d8fbfa2 > test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java > 020fa3f > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > c8fca68 > > test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java > 9aa69ed > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java > 9416473 > > test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java > 37a0a7d > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java > 4e46637 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java > 5bde35c > > test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromHDFSToKafkaTest.java > 88db2f2 > > test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java > 92a52b8 > > test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java > 5c90501 > test/src/test/resources/suite.xml PRE-CREATION > > Diff: https://reviews.apache.org/r/31824/diff/ > > > Testing > ------- > > Tested locally. Also, tested against SQOOP-1781 jira and posted results > there. They seem mixed largely because it's difficult to provide changes to > testing and changes to dev support together. > > > Thanks, > > Abraham Elmahrek > >
