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.

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.

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.

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

		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!

epugh commented Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants