Remove extra solr.xml and other config setup for tests that are not needed #3988

epugh · 2025-12-28T21:46:01Z

Description

Noticed some places where we had setup logic that wasn't needed.

Solution

Pruned it back!

Tests

Re ran tests

Copilot

Pull request overview

This PR removes unnecessary solr.xml file copying operations during test setup across multiple test classes, simplifying test initialization code.

Removed the unused copyMinFullSetup helper method from SolrTestCaseJ4
Eliminated solr.xml copying from test setup methods across multiple test files
Removed unused helper methods and imports that became redundant after cleanup

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java	Removed unused `copyMinFullSetup` method that copied solr.xml
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LB2SolrClientTest.java	Removed solr.xml copying from setUp and removed unused helper methods (getUrl, getConfDir, getSolrXmlFile)
solr/solrj/src/test/org/apache/solr/client/solrj/TestLBHttpSolrClient.java	Removed solr.xml copying from setUp and removed unused helper methods (getUrl, getConfDir, getSolrXmlFile)
solr/core/src/test/org/apache/solr/security/BasicAuthStandaloneTest.java	Removed solr.xml and config file copying from setUp, removed unused helper methods and constants, simplified Properties initialization
solr/core/src/test/org/apache/solr/schema/TestBinaryField.java	Removed solr.xml copying from beforeTest and removed unused SolrTestCaseJ4 import
solr/core/src/test/org/apache/solr/response/TestRawTransformer.java	Removed solr.xml copying from initStandalone, simplified Properties initialization, and removed unused SolrTestCaseJ4 import
solr/core/src/test/org/apache/solr/jersey/JerseyApplicationSharingTest.java	Removed unused collection and confDir constants
solr/core/src/test/org/apache/solr/handler/V2StandaloneTest.java	Removed solr.xml copying and unused Files import
solr/core/src/test/org/apache/solr/handler/TestRestoreCore.java	Removed solr.xml copying, simplified Properties initialization, and removed unnecessary "throws Exception" from test method signature
solr/core/src/test/org/apache/solr/handler/TestReplicationHandlerDiskOverFlow.java	Added System property configuration for URL allow-list checks
solr/core/src/test/org/apache/solr/handler/TestReplicationHandlerBackup.java	Removed solr.xml copying and simplified Properties initialization
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java	Removed solr.xml copying, simplified Properties initialization, and removed unused StandardCopyOption import
solr/core/src/test/org/apache/solr/TestCustomCoreProperties.java	Removed solr.xml copying, renamed variable from `src_dir` to `srcDir` for better Java naming convention, and simplified Properties initialization
solr/core/src/test/org/apache/solr/SolrTestCaseJ4Test.java	Removed solr.xml copying from beforeClass

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

epugh · 2025-12-29T15:52:12Z

@dsmiley I'd love a review of this...

dsmiley · 2025-12-30T14:53:32Z

solr/core/src/test/org/apache/solr/handler/TestReplicationHandlerDiskOverFlow.java

    System.clearProperty(TEST_URL_ALLOW_LIST);
+    System.clearProperty("solr.security.allow.urls.enabled");


Clearing properties in an @After or @AfterClass is always pointless since our test infra does that for us. Removing them everywhere would be a nice separate PR.

Good suggestion, will do that.

dsmiley · 2025-12-30T14:55:18Z

solr/core/src/test/org/apache/solr/handler/V2StandaloneTest.java

    Path solrHomeTmp = createTempDir();
    PathUtils.copyDirectory(
        TEST_HOME().resolve("configsets/minimal/conf"), solrHomeTmp.resolve("conf"));
-    Files.copy(TEST_HOME().resolve("solr.xml"), solrHomeTmp.resolve("solr.xml"));


So why is solr.xml copying no longer necessary?

Dunno. The test still runs. Sometimes I think that some of this code just got copy/pasted over and over from test to test. Maybe it mattered for one test, and then it just kept living like a zombie. If you are really itnerested I can investigate (i.e ask Little Buddy what the deal is!).

I think it's a bit irresponsible to do this on the grounds that tests pass, without looking into the matter any further. Take a look at solr/core/src/test-files/solr/solr.xml. There are comments there about why the settings are different (e.g. distribUpdateSoTimeout) and various customizability that (unfortunately) our standard solr.xml doesn't have (but should).

I can see that the current situation could use improvement. Probably a separate PR should tackle that. Maybe that solr.xml should be in the solr-test-framework JAR as a resource that the test infra can reference and be used by 3rd parties. But also need a simple one-liner to install it into provided Path dir. Or alternatively a new property to the node props which contains a path to where solr.xml is.

I noticed that I documented the rule's startSolr as follows, meaning it has good test defaults. But only EmbeddedSolrServerTestRule follows this. I suppose then we have room to improve the Jetty side, after which it will indeed make sense to not copy a general test solr.xml.

@dsmiley I have added https://issues.apache.org/jira/browse/SOLR-18053 to track your suggestion. I've also linked it to a VERY old JIRA of mine, https://issues.apache.org/jira/browse/SOLR-15788 which seeks to get us away from the kitchen sink approach as well.

dsmiley · 2025-12-30T14:56:55Z

solr/core/src/test/org/apache/solr/security/BasicAuthStandaloneTest.java

-  private static final Path CONF_DIR =
-      ROOT_DIR.resolve("configsets").resolve("configset-2").resolve("conf");


It appears you've changed this test to no longer use configset-2. Why?

I think I removed the vairuable and it still ran... I went and looked and yeah, they pretty much are the same:

Solrconfig files: Nearly identical except for a comment difference: configset-1: Generic comment about being a "kitchen sink" config file for tests configset-2: Specific comment stating "Small solrconfig with no /get handler defined, for use in TestConfigSets#testConfigSetOnReload"

Lets not change this in this PR.
"collection1" is the default, not "configset-1". It is an unfortunate kitchen-sink, with many tests using it. I'd rather see tests use one of several basic / standard configs without any quirks and then if needed modify it at runtime using config edit API or another approach on the fly.

Okay, I've backed out the change to this class... I think moving this class to using the kitchen sink collection1 is better than just randomly using configset-2, which was meant for only the TestConfigSets class, but I'd rather get this PR in! I do agree we need to deal with the kitchen-sink issue!

I looked into the history of configset2 when I wrote my comment. I see it as a general addition for something simple. It has a bad name! Other tests now use it too. I didn't see it as a configset that was designed fundamentally and only for TestConfigSets. Maybe I overlooked something... why do you think it's only suitable for that test?

dsmiley · 2025-12-30T15:06:39Z

solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java

-        Path.of(instance.getHomeDir(), "solr.xml"),
-        StandardCopyOption.REPLACE_EXISTING);
-    Properties nodeProperties = new Properties();
-    nodeProperties.setProperty("solr.data.dir", instance.getDataDir());


I guess generally you have identified that setting solr.data.dir isn't necessary?

yeah... I ran the test before and after the cahnge and everythign was green. Then I started looking for other tests that had solr.data.dir and did the same thing...

I at least checked where SolrInstance is used, data dir is the default so, indeed, at least where SolrInstance is used (like here), we don't need to relay it to "solr.data.dir".

…ml_copy

…class!

…ml_copy

epugh · 2025-12-30T23:28:39Z

Okay, we got to green! I'd love a +1. I will wait till I figure out what is causing builds in main to fail on DistributedDebugComponentTest first however.

…ml_copy

…nc process

epugh · 2026-01-03T13:15:41Z

@dsmiley with the various changes I've made today, thoughts on being able to merge this?

dsmiley

I don't think this should be merged until you've validated these tests work with SSL mode, or say generally a higher tests multiplier. Probably requires #4008

Can you please remove

solr/solr/core/src/test/org/apache/solr/handler/admin/MetricsHandlerTest.java

Line 40 in 0efab55

h.getCoreContainer().waitForLoadingCoresToFinish(30000);

which is the only other call to waitForLoadingCoresToFinish that seems pointless?

dsmiley · 2026-01-04T03:36:11Z

solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java

@@ -2259,12 +2258,6 @@ public static void copyMinConf(Path dstRoot, String propertiesContent, String so
        subHome.resolve("solrconfig.snippet.randomindexconfig.xml"));
  }

-  // Just copies the file indicated to the tmp home directory naming it "solr.xml"


I hate this method; thanks for removing it.

dsmiley · 2026-01-04T03:43:34Z

solr/core/src/test/org/apache/solr/schema/TestBinaryField.java

-    Files.copy(Path.of(src_dir, "schema-binaryfield.xml"), confDir.resolve("schema.xml"));
-    Files.copy(Path.of(src_dir, "solrconfig-basic.xml"), confDir.resolve("solrconfig.xml"));
+    // Copy the custom schema for binary field tests
+    String sourceConfDir = TEST_HOME() + "/collection1/conf";


(okay to defer this; problem pre-existed)
If you look carefully at what's happening here (was before), this is sad. TEST_HOME() returns a Path, which we toString via string concatenation to a String representing a path, and then a line below we create a Path via Path.of. Obviously we should instead be using the resolve method on Path. The tell-tale sign of the problem is the usage of Path.of which we should ideally use sparingly (when we truly have a String input that can't be a Path).

CC @mlbiscoc

epugh added 5 commits December 28, 2025 15:09

We copy extra solr.xml around that aren't needed to be copied

ee7ebed

various setup calls that are NOT needed

4079397

Unused variables

91d2f92

Removing unused settings

e8b8bca

Need to disable the allow.urls to make test pass.

e10ce96

epugh requested a review from Copilot December 28, 2025 21:46

github-actions bot added test-framework client:solrj tests cat:search cat:schema cat:security labels Dec 28, 2025

epugh changed the title ~~Remove extra solr xml coping during test setup.~~ Remove extra solr xml and other configuraiton steps during test setup that are not needed Dec 28, 2025

epugh added the no-changelog label Dec 28, 2025

Copilot started reviewing on behalf of epugh December 28, 2025 21:46 View session

Copilot AI reviewed Dec 28, 2025

View reviewed changes

epugh requested a review from dsmiley December 29, 2025 15:51

dsmiley reviewed Dec 30, 2025

View reviewed changes

epugh added 7 commits December 30, 2025 13:41

Merge remote-tracking branch 'upstream/main' into remove_extra_solr_x…

427b35e

…ml_copy

Remove unused property

150995b

Remove method that A) doesn't do anything and B) only used by one sub…

21b14cc

…class!

Remove unneeded piece of test setup

8330f9c

Merge remote-tracking branch 'upstream/main' into remove_extra_solr_x…

14ccb3a

…ml_copy

lint

f14e67f

fix merge issue

f8d6c93

epugh changed the title ~~Remove extra solr xml and other configuraiton steps during test setup that are not needed~~ Remove extra solr.xml and other config setup for tests that are not needed Dec 30, 2025

epugh added 2 commits January 3, 2026 07:18

Merge remote-tracking branch 'upstream/main' into remove_extra_solr_x…

da259ba

…ml_copy

audited use of this command and removed for where core creation is sy…

56f2c8b

…nc process

This was referenced Jan 3, 2026

Migrate Solr tests from SolrJettyTestBase to using SolrJettyTestRule #3947

Merged

SOLR-10654: Introduce output of Prometheus metrics directly from Solr #2405

Merged

dsmiley reviewed Jan 4, 2026

View reviewed changes

		System.clearProperty(TEST_URL_ALLOW_LIST);
		System.clearProperty("solr.security.allow.urls.enabled");

		private static final Path CONF_DIR =
		ROOT_DIR.resolve("configsets").resolve("configset-2").resolve("conf");

Remove extra solr.xml and other config setup for tests that are not needed #3988

Are you sure you want to change the base?

Remove extra solr.xml and other config setup for tests that are not needed #3988

Conversation

epugh commented Dec 28, 2025

Description

Solution

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

epugh commented Dec 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epugh commented Dec 30, 2025

Uh oh!

epugh commented Jan 3, 2026

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants