You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/02/18 18:49:08 UTC
[GitHub] [solr] HoustonPutman opened a new pull request #654: SOLR-6443: Cleanup managedResourceTest
HoustonPutman opened a new pull request #654:
URL: https://github.com/apache/solr/pull/654
Mainly remove the need for a SolrCore in the test.
https://issues.apache.org/jira/browse/SOLR-6443
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #654: SOLR-6443: Cleanup managedResourceTest
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #654:
URL: https://github.com/apache/solr/pull/654#discussion_r810266482
##########
File path: solr/core/src/test/org/apache/solr/rest/TestManagedResourceStorage.java
##########
@@ -36,28 +35,19 @@
/**
* Depends on ZK for testing ZooKeeper backed storage logic.
*/
-@Slow
-// commented 4-Sep-2018 @LuceneTestCase.BadApple(bugUrl = "https://issues.apache.org/jira/browse/SOLR-6443")
public class TestManagedResourceStorage extends AbstractZkTestCase {
/**
* Runs persisted managed resource creation and update tests on Zookeeper storage.
*/
@Test
public void testZkBasedJsonStorage() throws Exception {
-
- // test using ZooKeeper
- assertTrue("Not using ZooKeeper", h.getCoreContainer().isZooKeeperAware());
- SolrResourceLoader loader = new SolrResourceLoader(Paths.get("./"));
// Solr unit tests can only write to their working directory due to
// a custom Java Security Manager installed in the test environment
- NamedList<String> initArgs = new NamedList<>();
- try {
+ try (SolrResourceLoader loader = new SolrResourceLoader(Paths.get("./"))) {
Review comment:
Should this be local directory or `createTempDir` like in the next test?
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/AbstractZkTestCase.java
##########
@@ -62,39 +51,22 @@ public static void azt_beforeClass() throws Exception {
System.setProperty("jetty.port", "0000");
System.setProperty(ZOOKEEPER_FORCE_SYNC, "false");
- zkServer.buildZooKeeper(SOLRHOME,
+ zkServer.buildZooKeeper(SolrTestCaseJ4.TEST_PATH(),
Review comment:
I wouldn't inline this... at worst, copy `getSolrHome` from BaseDistributesSearchTestCase and use that. The problem is just like the comment up above says - if we're calling this from solrj or from a third party that uses our test-framework (and this is technically publicly available) then TEST_PATH won't work and they'll have no way to override/inject the correct value for their installation.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #654: SOLR-6443: Cleanup managedResourceTest
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #654:
URL: https://github.com/apache/solr/pull/654#discussion_r828276495
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/AbstractZkTestCase.java
##########
@@ -62,39 +51,22 @@ public static void azt_beforeClass() throws Exception {
System.setProperty("jetty.port", "0000");
System.setProperty(ZOOKEEPER_FORCE_SYNC, "false");
- zkServer.buildZooKeeper(SOLRHOME,
+ zkServer.buildZooKeeper(SolrTestCaseJ4.TEST_PATH(),
Review comment:
So `AbstractZkTestCase` is only used by `TestManagedResourceStorage`, which doesn't actually use the configs stored in Zookeeper, so we can go-ahead and just remove this 🙂
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #654: SOLR-6443: Cleanup managedResourceTest
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #654:
URL: https://github.com/apache/solr/pull/654#discussion_r826528588
##########
File path: solr/core/src/test/org/apache/solr/rest/TestManagedResourceStorage.java
##########
@@ -116,7 +103,7 @@ private void doStorageTests(SolrResourceLoader loader, StorageIO storageIO) thro
assertEquals("true", storedArgs.get("ignoreCase"));
List<String> storedList = (List<String>)storedMap.get(ManagedResource.MANAGED_JSON_LIST_FIELD);
assertNotNull(storedList);
- assertTrue(storedList.size() == managedList.size());
+ assertEquals(storedList.size(), managedList.size());
Review comment:
nit: I think managedList is the expected value, swap the order of these arguments.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #654: SOLR-6443: Cleanup managedResourceTest
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #654:
URL: https://github.com/apache/solr/pull/654#discussion_r828262464
##########
File path: solr/core/src/test/org/apache/solr/rest/TestManagedResourceStorage.java
##########
@@ -36,28 +35,19 @@
/**
* Depends on ZK for testing ZooKeeper backed storage logic.
*/
-@Slow
-// commented 4-Sep-2018 @LuceneTestCase.BadApple(bugUrl = "https://issues.apache.org/jira/browse/SOLR-6443")
public class TestManagedResourceStorage extends AbstractZkTestCase {
/**
* Runs persisted managed resource creation and update tests on Zookeeper storage.
*/
@Test
public void testZkBasedJsonStorage() throws Exception {
-
- // test using ZooKeeper
- assertTrue("Not using ZooKeeper", h.getCoreContainer().isZooKeeperAware());
- SolrResourceLoader loader = new SolrResourceLoader(Paths.get("./"));
// Solr unit tests can only write to their working directory due to
// a custom Java Security Manager installed in the test environment
- NamedList<String> initArgs = new NamedList<>();
- try {
+ try (SolrResourceLoader loader = new SolrResourceLoader(Paths.get("./"))) {
Review comment:
probably a temporary directory
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org