> On Nov. 24, 2014, 3:15 p.m., Veena Basavaraj wrote: > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java, > > line 126 > > <https://reviews.apache.org/r/28373/diff/1/?file=773803#file773803line126> > > > > can we have any more assert here to say that the upgrade actually > > kicked off, some version assertion? > > Jarek Cecho wrote: > I was thinking about that and sadly I currently don't have a good way how > to directly detect whether the upgrade happened. We know that it happened > only based on the invaritants that are true when you get to the > testPostUpgrade() method call. The invariant is that the server is up and > running and as a result if we started with older repository version, it had > to be upgraded (otherwise the server would fail to start). I recognize that > this is not the ideal state, but I don't have anything better available at > the moment.
Please add a todo and follow up to do so, it is a good start but still needs some more work to guard against bad code > On Nov. 24, 2014, 3:15 p.m., Veena Basavaraj wrote: > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java, > > line 46 > > <https://reviews.apache.org/r/28373/diff/1/?file=773803#file773803line46> > > > > Will we add a test per repository? in that case please create a ticket > > for post gres since that is important to have before we call the post gres > > repo work done. > > > > > > Please rename this class to reflect the below, > > > > It is not testing repository schema upgrade, it should be exercising > > the upgradeConnector nd upgradeDriver code in the Respository.java. > > > > So please plug the word data into. > > > > Please add these comments explaining what this test case does, so > > someone new can really understand it > > > > DerbyRepoDataUpgrade seems more apt. > > Jarek Cecho wrote: > Yes, I would assume that we will end up having different tests for > different repository implementations. Considering that different repository > implementations will have different physical structure and requirementes, I > believe that it's expected. E.g. I'm assuming that for PostgreSQL we will > have to do pg_dump instead of simply targzing the repository structures. > > We can't add PostgreSQL upgrade tests, because PostgreSQL haven't been > release yet and hence there is nothing to upgrade from. I'll however open > subsequent JIRA to not forget on this one. I would also like to see a step in > our "How to release wiki page" that will request creation of such JIRAs post > every release, so that we will have new tests going forward as well. > > This test actually exercise both schema changes and data changes. The > idea is to have a snapshot of previous version and verify that I can get from > the previous version all the way to current state. And this might (and will) > include both structural changes and data changes. Hence If you don't mind, I > would like to keep the name as it is. I actuall said please create a ticket for post gres. Not a test.!! So filing a JIRA as a sub task of the post gres task would be not hard. https://issues.apache.org/jira/browse/SQOOP-1523 If it is testing both, lets make it explicit in a comment and or the name of the class. Quoting from the kafka guide: http://kafka.apache.org/coding-guide.html Clear code is preferable to comments. When possible make your naming so good you don't need comments. When that isn't possible comments should be thought of as mandatory, write them to be read. > On Nov. 24, 2014, 3:15 p.m., Veena Basavaraj wrote: > > test/src/main/java/org/apache/sqoop/test/minicluster/SqoopMiniCluster.java, > > line 214 > > <https://reviews.apache.org/r/28373/diff/1/?file=773799#file773799line214> > > > > rename this to connectorUpgradeProperty ( configuration is overloaded > > term in sqoop_ > > > > It is not a connector configuration. I understand why you have named it > > this way since there are other places, if you can possiblu change all > > others its good. > > Jarek Cecho wrote: > It seems to me that your comment is not for this line, but for the line > above, correct? I'm assuming that you would prefer to rename the newly > introduced method getConnectorConfiguration()? > > If so then, I would suggest to use connectorManagerConfiguration() as > that is more descriptive. Even though that the default method implementation > is configuring only the upgrade, other ConnectorManager configuration > properties could be specified there. sure Manager works. - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28373/#review62894 ----------------------------------------------------------- On Nov. 22, 2014, 6:24 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28373/ > ----------------------------------------------------------- > > (Updated Nov. 22, 2014, 6:24 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1783 > https://issues.apache.org/jira/browse/SQOOP-1783 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > I've added automated test as suggested on the JIRA. > > You need to put archive derby-repository-1.99.4.tar.gz that is attached to > JIRA to path > test/src/test/resources/repository/derby/derby-repository-1.99.4.tar.gz in > order to verify this patch. > > > Diffs > ----- > > pom.xml e626c7d > test/pom.xml cafa250 > test/src/main/java/org/apache/sqoop/test/minicluster/SqoopMiniCluster.java > 4322b1c > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java > 5e1d564 > test/src/main/java/org/apache/sqoop/test/utils/CompressionUtils.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_4UpgradeTest.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java > PRE-CREATION > test/src/test/resources/repository/derby/derby-repository-1.99.4.tar.gz > PRE-CREATION > > Diff: https://reviews.apache.org/r/28373/diff/ > > > Testing > ------- > > The new test is passing! > > > Thanks, > > Jarek Cecho > >
