> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote: > > 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.
Yeah good point. I can dig a bit to find out what we can do here. > On April 21, 2015, 2:13 a.m., Jarek Cecho wrote: > > test/src/test/resources/suite.xml, line 21 > > <https://reviews.apache.org/r/31824/diff/3/?file=934932#file934932line21> > > > > 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? Yep. Good point. > On April 21, 2015, 2:13 a.m., Jarek Cecho wrote: > > test/src/test/resources/suite.xml, lines 25-29 > > <https://reviews.apache.org/r/31824/diff/3/?file=934932#file934932line25> > > > > 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. Yep. Good point. > On April 21, 2015, 2:13 a.m., Jarek Cecho wrote: > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java, > > lines 49-177 > > <https://reviews.apache.org/r/31824/diff/3/?file=934927#file934927line49> > > > > Seems as a lot of unrelated changes and moves around? Actually, "beforeMethod" was completely removed because it wasn't being executed in time any more. The test name will be context.getName() until TomcatTestCase.startServer is ran. I did move getTestName() unintentionally. It took me a while to get any of this working correctly. Let me know if you'd like to move getTestName() back to its original place. > On April 21, 2015, 2:13 a.m., Jarek Cecho wrote: > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java, > > line 46 > > <https://reviews.apache.org/r/31824/diff/3/?file=934927#file934927line46> > > > > It seems that we're re-classifying this test, so I'm wondering whether > > this should be done in separate JIRA? Yep. > On April 21, 2015, 2:13 a.m., Jarek Cecho wrote: > > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java, > > lines 98-103 > > <https://reviews.apache.org/r/31824/diff/3/?file=934925#file934925line98> > > > > 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. Indeed. I've changed the getTestName() methods in the other classes as well. Now, it uses context.getName() everywhere to ensure that when this method is called, there won't be an NPE. Maybe we can move it higher up in the inheritance chain? > On April 21, 2015, 2:13 a.m., Jarek Cecho wrote: > > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java, > > line 108 > > <https://reviews.apache.org/r/31824/diff/3/?file=934925#file934925line108> > > > > We're changing the default value here, so I'm wondering if it's > > intentional? Completely intentional. ConnectorTestCase was overriding this and using a different default. I've removed the method overriding this one in ConnectorTestCase and changed the default here. > On April 21, 2015, 2:13 a.m., Jarek Cecho wrote: > > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java, > > lines 82-94 > > <https://reviews.apache.org/r/31824/diff/3/?file=934923#file934923line82> > > > > I'm wondering why we are removing this one? It turns out overriding a method that is annotated with @BeforeSuite is not correct. So I've removed it and am using HadoopMiniClusterRunner.class as the default in TomcatTestCase. > On April 21, 2015, 2:13 a.m., Jarek Cecho wrote: > > test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java, > > line 44 > > <https://reviews.apache.org/r/31824/diff/3/?file=934924#file934924line44> > > > > 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? Actually, I'm not sure. In order to get the kafka tests working I had to play around with a few different mechanisms: topic name and zookeeper configuration. I might be able to remove this given the Zookeeper configuration changes. I'll check. > On April 21, 2015, 2:13 a.m., Jarek Cecho wrote: > > test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java, > > line 27 > > <https://reviews.apache.org/r/31824/diff/3/?file=934922#file934922line27> > > > > Is this intentional change? not at all :) - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31824/#review80876 ----------------------------------------------------------- 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 > >
