> On April 26, 2015, 8:07 p.m., Jarek Cecho wrote: > > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java, > > lines 86-91 > > <https://reviews.apache.org/r/33570/diff/1/?file=942028#file942028line86> > > > > I'm wondering why we are changing the visilibyt from private to > > protected? > > > > (I understand the addition of "static") > > Abraham Elmahrek wrote: > DerbyRepositoryUpgradeTest requires this for now.
Can't we just create a setSqoopClient() method instead? Seems as cleaner approach then have this variable protected. > On April 26, 2015, 8:07 p.m., Jarek Cecho wrote: > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java, > > lines 134-142 > > <https://reviews.apache.org/r/33570/diff/1/?file=942029#file942029line134> > > > > I do not particularly like this pattern when we are prohibit parent > > class to do something that it was designed to do. Do you think that it > > would make sense to split the TomcatTestCase to two classes: > > > > * TomcatTestCaseBase that will contain all variables/help methods but > > won't do any action. Then this DerbyRepositoryUpgradeTest class will > > inherit from the new base class. > > * TomatTestCase that will inherit from Base and will add the automatic > > actions of starting/stopping required runners. > > > > I know that you're thinking about improving the integration test suite > > by using annotations to start/stop only those runners that are required, so > > perhaps my note is not so much relevant as this is just a "temporary" > > workaround. > > Abraham Elmahrek wrote: > I was thinking the exact same thing. I do plan on changing this > drastically. Let's leave this and change it in the immediate future? Fine with me considering that this is one class. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33570/#review81633 ----------------------------------------------------------- On April 26, 2015, 8:19 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33570/ > ----------------------------------------------------------- > > (Updated April 26, 2015, 8:19 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1953 > https://issues.apache.org/jira/browse/SQOOP-1953 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit 1250abe418f343ab0a2ffa010a2a15a97befb385 > Author: Abraham Elmahrek <[email protected]> > Date: Fri Apr 17 17:34:15 2015 -0700 > > SQOOP-1953: Tomcat in suite > > :100644 100644 bc2bec7... 6e5e038... M pom.xml > :100644 100644 98a60f6... a9502d2... M test/pom.xml > :100644 100644 4d27886... 5a6773d... M > test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java > :100644 100644 6729cc7... 197dab4... M > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java > :100644 100644 a687c16... 7ad3dc2... M > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgrade > :100644 100644 101b6ec... f0dd905... M > test/src/test/resources/integration-tests-suite.xml > :000000 100644 0000000... 2856556... A > test/src/test/resources/upgrade-tests-suite.xml > > > Diffs > ----- > > pom.xml bc2bec7 > test/pom.xml 98a60f6 > > test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java > 4d27886 > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java > 6729cc7 > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java > a687c16 > test/src/test/resources/integration-tests-suite.xml 101b6ec > test/src/test/resources/upgrade-tests-suite.xml PRE-CREATION > > Diff: https://reviews.apache.org/r/33570/diff/ > > > Testing > ------- > > > Thanks, > > Abraham Elmahrek > >
