Deal with properties that are needed for ONE test, but pollute all our tests. (#3987)

This PR addresses test pollution by isolating system property usage that was only needed by a single test but was being set globally across 33 different test locations. The solution creates a dedicated test class TestConfigPropertySubstitution for the one test that actually requires the solr.test.sys.prop1 and solr.test.sys.prop2 properties, and removes the unnecessary property setup/teardown from 32 other test locations.

Author: epugh

solr/core/src/test-files/solr/collection1/conf/solrconfig-analytics-query.xml

@@ -217,10 +217,6 @@
     </httpCaching>
   </requestDispatcher>
 
-  <!-- test getting system property -->
-  <propTest attr1="${solr.test.sys.prop1}-$${literal}"
-            attr2="${non.existent.sys.prop:default-from-config}">prefix-${solr.test.sys.prop2}-suffix</propTest>
-
   <queryParser name="count" class="org.apache.solr.search.AnalyticsTestQParserPlugin"/>
 
   <updateRequestProcessorChain name="uniq-fields">

solr/core/src/test-files/solr/collection1/conf/solrconfig-collapseqparser.xml

@@ -220,10 +220,6 @@
     </httpCaching>
   </requestDispatcher>
 
-  <!-- test getting system property -->
-  <propTest attr1="${solr.test.sys.prop1}-$${literal}"
-            attr2="${non.existent.sys.prop:default-from-config}">prefix-${solr.test.sys.prop2}-suffix</propTest>
-
   <queryParser name="foo" class="FooQParserPlugin"/>
 
   <updateRequestProcessorChain name="uniq-fields">

solr/core/src/test-files/solr/collection1/conf/solrconfig-elevate.xml

@@ -120,10 +120,6 @@
     </httpCaching>
   </requestDispatcher>
 
-  <!-- test getting system property -->
-  <propTest attr1="${solr.test.sys.prop1}-$${literal}"
-            attr2="${non.existent.sys.prop:default-from-config}">prefix-${solr.test.sys.prop2}-suffix</propTest>
-
   <queryParser name="foo" class="FooQParserPlugin"/>
 
   <initParams path="/elevate,/dataElevate">

solr/core/src/test-files/solr/collection1/conf/solrconfig-minhash.xml

@@ -434,10 +434,6 @@
     </lst>
   </requestHandler>
 
-  <!-- test getting system property -->
-  <propTest attr1="${solr.test.sys.prop1}-$${literal}"
-            attr2="${non.existent.sys.prop:default-from-config}">prefix-${solr.test.sys.prop2}-suffix</propTest>
-
   <queryParser name="foo" class="FooQParserPlugin"/>
 
   <updateRequestProcessorChain name="uniq-fields">

solr/core/src/test-files/solr/collection1/conf/solrconfig-plugcollector.xml

@@ -430,10 +430,6 @@
     </httpCaching>
   </requestDispatcher>
 
-  <!-- test getting system property -->
-  <propTest attr1="${solr.test.sys.prop1}-$${literal}"
-            attr2="${non.existent.sys.prop:default-from-config}">prefix-${solr.test.sys.prop2}-suffix</propTest>
-
   <queryParser name="rank" class="org.apache.solr.search.RankQueryTestPlugin"/>
 
   <updateRequestProcessorChain name="uniq-fields">

solr/core/src/test-files/solr/collection1/conf/solrconfig-test-misc.xml

@@ -28,14 +28,8 @@
   <directoryFactory name="DirectoryFactory" class="${solr.directoryFactory:solr.MockDirectoryFactory}"/>
   <schemaFactory class="ClassicIndexSchemaFactory"/>
 
-  <!-- see TestConfig.testJavaProperty -->
-  <propTest attr1="${solr.test.sys.prop1}-$${literal}"
-            attr2="${non.existent.sys.prop:default-from-config}">prefix-${solr.test.sys.prop2}-suffix</propTest>
-
   <!-- see TestConfig.testDisableRequetsHandler -->
   <requestHandler name="/disabled" class="solr.SearchHandler" enable="false"/>
   <requestHandler name="/enabled" class="solr.SearchHandler" enable="true"/>
 
-
-
 </config>

solr/core/src/test-files/solr/collection1/conf/solrconfig-test-properties.xml

@@ -0,0 +1,37 @@
+<?xml version="1.0" ?>
+
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements.  See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<!-- 
+  A minimal solrconfig specifically for testing property substitution functionality.
+  This config is used by TestConfigPropertySubstitution.testJavaPropertySubstitution() to test
+  that system property substitution works correctly in Solr configuration files.
+-->
+<config>
+  <luceneMatchVersion>${tests.luceneMatchVersion:LATEST}</luceneMatchVersion>
+  <dataDir>${solr.data.dir:}</dataDir>
+  <xi:include href="solrconfig.snippet.randomindexconfig.xml" xmlns:xi="http://www.w3.org/2001/XInclude"/>
+  <directoryFactory name="DirectoryFactory" class="${solr.directoryFactory:solr.MockDirectoryFactory}"/>
+  <schemaFactory class="ClassicIndexSchemaFactory"/>
+  
+  <!-- This element is specifically for testing property substitution -->
+  <propTest attr1="${solr.test.sys.prop1}-$${literal}"
+            attr2="${non.existent.sys.prop:default-from-config}">prefix-${solr.test.sys.prop2}-suffix</propTest>
+
+  <requestHandler name="/select" class="solr.SearchHandler" />
+</config>

solr/core/src/test-files/solr/collection1/conf/solrconfig.xml

@@ -467,10 +467,6 @@
     </lst>
   </requestHandler>
 
-  <!-- test getting system property -->
-  <propTest attr1="${solr.test.sys.prop1}-$${literal}"
-            attr2="${non.existent.sys.prop:default-from-config}">prefix-${solr.test.sys.prop2}-suffix</propTest>
-
   <queryParser name="foo" class="FooQParserPlugin"/>
 
   <updateRequestProcessorChain name="uniq-fields">

solr/core/src/test/org/apache/solr/cloud/TestRequestForwarding.java

@@ -35,18 +35,13 @@ public class TestRequestForwarding extends SolrTestCaseJ4 {
   @Override
   public void setUp() throws Exception {
     super.setUp();
-    System.setProperty("solr.test.sys.prop1", "propone");
-    System.setProperty("solr.test.sys.prop2", "proptwo");
     solrCluster = new MiniSolrCloudCluster(3, createTempDir(), JettyConfig.builder().build());
     solrCluster.uploadConfigSet(TEST_PATH().resolve("collection1/conf"), "conf1");
   }
 
   @Override
   public void tearDown() throws Exception {
     solrCluster.shutdown();
-    System.clearProperty("solr.test.sys.prop1");
-    System.clearProperty("solr.test.sys.prop2");
-
     super.tearDown();
   }
 

solr/core/src/test/org/apache/solr/core/TestConfig.java

@@ -20,13 +20,11 @@
 import java.io.InputStream;
 import java.util.Collections;
 import java.util.LinkedHashMap;
-import java.util.List;
 import org.apache.lucene.index.ConcurrentMergeScheduler;
 import org.apache.lucene.index.IndexWriterConfig;
 import org.apache.lucene.index.TieredMergePolicy;
 import org.apache.lucene.util.InfoStream;
 import org.apache.solr.SolrTestCaseJ4;
-import org.apache.solr.common.ConfigNode;
 import org.apache.solr.schema.IndexSchema;
 import org.apache.solr.schema.IndexSchemaFactory;
 import org.apache.solr.search.CacheConfig;
@@ -63,30 +61,6 @@ public void testDisableRequetsHandler() {
     assertNotNull(h.getCore().getRequestHandler("/enabled"));
   }
 
-  @Test
-  public void testJavaProperty() {
-    // property values defined in build.xml
-
-    String s = solrConfig.get("propTest").txt();
-    assertEquals("prefix-proptwo-suffix", s);
-
-    s = solrConfig.get("propTest").attr("attr1", "default");
-    assertEquals("propone-${literal}", s);
-
-    s = solrConfig.get("propTest").attr("attr2", "default");
-    assertEquals("default-from-config", s);
-
-    assertEquals(
-        "prefix-proptwo-suffix",
-        solrConfig.get("propTest", it -> "default-from-config".equals(it.attr("attr2"))).txt());
-
-    List<ConfigNode> nl = solrConfig.root.getAll("propTest");
-    assertEquals(1, nl.size());
-    assertEquals("prefix-proptwo-suffix", nl.get(0).txt());
-
-    assertEquals("prefix-proptwo-suffix", solrConfig.get("propTest").txt());
-  }
-
   @Test
   public void testCacheEnablingDisabling() throws Exception {
     // ensure if cache is not defined in the config then cache is disabled