thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-501586703 > PR looks good to me. Sorry for my tardiness in reviewing it. > > I have some qualms about introducing a new test base class entirely as this PR does. Our test-base situation is already quite jumbled and there's a lot of developer confusion around when each test-case should be used. There's also been some recent movement towards randomizing the SolrJ RequestWriter in one of the existing test bases. So it's hard to say whether adding a new test-base-class will hold up long term. But I don't want to let the perfect get in the way of the good here - especially since this PR adds some awesome test coverage for atomic updates of a large variety of field types. > > If Noble gets a chance to review #681 I'll circle back and merge this. (Assuming he doesn't beat me to it). You are right about the number of existing test bases, but from my point of view SolrJettyTestBase is hiding the usage of EmbeddedSolrServer, which no one is expecting to be used there. So i think it is clearer now. To get rid of this situation the whole test base have to be re-written, which is to much for this PR ;-)
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
