gerlowskija commented on code in PR #1508:
URL: https://github.com/apache/solr/pull/1508#discussion_r1194268355
##########
solr/solrj-zookeeper/src/test/org/apache/solr/common/cloud/SolrZkClientTest.java:
##########
@@ -281,4 +282,42 @@ public void testCheckInterrupted() {
SolrZkClient.checkInterrupted(new InterruptedException());
assertTrue(Thread.currentThread().isInterrupted());
}
+
+ @Test
+ public void testWithSolrResourceLoader() {
+ setupSystemProperties();
+ SolrResourceLoader solrResourceLoader =
+ new SolrResourceLoader(Path.of("."),
ClassLoader.getSystemClassLoader());
+ new SolrZkClient.Builder()
+ .withUrl(zkServer.getZkHost())
+ .withTimeout(AbstractZkTestCase.TIMEOUT, TimeUnit.MILLISECONDS)
+ .withSolrClassLoader(solrResourceLoader)
+ .build()
+ .close(); // no more tests needed. We only test class instantiation
+ clearSystemProperties();
+ }
+
+ private static void setupSystemProperties() {
Review Comment:
[0] Small nit: this might benefit from a name that describes what sysprops
you're setting up. e.g. `enableCustomCredentialsProvider` or something similar.
But if you like that name as-is, that's also fine.
##########
solr/solrj-zookeeper/src/test/org/apache/solr/common/cloud/SolrZkClientTest.java:
##########
@@ -281,4 +282,42 @@ public void testCheckInterrupted() {
SolrZkClient.checkInterrupted(new InterruptedException());
assertTrue(Thread.currentThread().isInterrupted());
}
+
+ @Test
+ public void testWithSolrResourceLoader() {
+ setupSystemProperties();
+ SolrResourceLoader solrResourceLoader =
+ new SolrResourceLoader(Path.of("."),
ClassLoader.getSystemClassLoader());
+ new SolrZkClient.Builder()
+ .withUrl(zkServer.getZkHost())
+ .withTimeout(AbstractZkTestCase.TIMEOUT, TimeUnit.MILLISECONDS)
+ .withSolrClassLoader(solrResourceLoader)
+ .build()
+ .close(); // no more tests needed. We only test class instantiation
+ clearSystemProperties();
Review Comment:
[Q] Do you know whether these system properties actually need cleared? Solr
tests all run with a JUnit '@Rule' (called "SystemPropertiesRestoreRule") that
is supposed to clear them in between each test case.
If that's not working, I can file a separate ticket to fix that. But if it
is, we should remove redundant sysprop removal here.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
