Skip to content

Remove unneeded clearing of system properties #4010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
epugh wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Remove unneeded clearing of system properties #4010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6a6ace7
Remove unneeded clearing of system properties
epugh Jan 5, 2026
f09fb5c
Merge remote-tracking branch 'upstream/main' into investigate_removin…
epugh Jan 5, 2026
7415ef0
Test failed due to having a very low zookeeper watch limit, and so th…
epugh Jan 5, 2026
af0a499
Deal with floating point precision differences.
epugh Jan 6, 2026
8851747
Review each test to remove unneeded clearing of properties, and remov…
epugh Jan 7, 2026
862d2f9
Merge remote-tracking branch 'upstream/main' into investigate_removin…
epugh Jan 7, 2026
fe7359a
Missed a method
epugh Jan 8, 2026
2efdc16
responding to reviewer feedback
epugh Jan 8, 2026
f9123b7
Remove empty After/AfterClass methods.
epugh Jan 8, 2026
fb6d205
Empty finally block
epugh Jan 8, 2026
f4e359a
Restoring this test back to original, the clear property, helps conve…
epugh Jan 8, 2026
f29f0e3
Restore changes to properties related to test setup (not tear down)
epugh Jan 10, 2026
605675d
Back out changes that were focused on fixing flaky tests for another day
epugh Jan 10, 2026
29c2578
Explicit property clearning that is not needed
epugh Jan 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions solr/core/src/test/org/apache/solr/AnalysisAfterCoreReloadTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.apache.solr.client.solrj.request.UpdateRequest;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.core.SolrCore;
import org.junit.AfterClass;
import org.junit.BeforeClass;

public class AnalysisAfterCoreReloadTest extends SolrTestCaseJ4 {
Expand All @@ -46,9 +45,6 @@ public static void beforeClass() throws Exception {
initCore("solrconfig.xml", "schema.xml", tmpSolrHome);
}

@AfterClass
public static void AfterClass() {}

public void testStopwordsAfterCoreReload() throws Exception {
SolrInputDocument doc = new SolrInputDocument();
doc.setField("id", "42");
Expand Down
4 changes: 0 additions & 4 deletions solr/core/src/test/org/apache/solr/SolrTestCaseJ4Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import java.nio.file.Path;
import org.apache.commons.io.file.PathUtils;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;

Expand Down Expand Up @@ -50,9 +49,6 @@ public static void beforeClass() throws Exception {
initCore("solrconfig-minimal.xml", "schema-tiny.xml", tmpSolrHome, "core1");
}

@AfterClass
public static void AfterClass() {}

@Test
public void testCorrectCore() {
assertEquals("should be core1", "core1", h.getCore().getName());
Expand Down
6 changes: 0 additions & 6 deletions solr/core/src/test/org/apache/solr/TestGroupingSearch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import org.apache.solr.response.ResultContext;
import org.apache.solr.response.SolrQueryResponse;
import org.apache.solr.schema.IndexSchema;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
Expand Down Expand Up @@ -74,11 +73,6 @@ public static void beforeTests() throws Exception {
initCore("solrconfig.xml", "schema12.xml");
}

@AfterClass
public static void afterTests() {
systemClearPropertySolrTestsMergePolicyFactory();
}

@Before
public void cleanIndex() {
assertU(delQ("*:*"));
Expand Down
8 changes: 2 additions & 6 deletions solr/core/src/test/org/apache/solr/TestHighlightDedupGrouping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.common.SolrInputDocument;
import org.junit.AfterClass;
import org.junit.Test;

/**
Expand All @@ -34,9 +33,6 @@ public class TestHighlightDedupGrouping extends BaseDistributedSearchTestCase {
private static final String group_ti1 = "group_ti1";
private static final String shard_i1 = "shard_i1";

@AfterClass
public static void afterClass() throws Exception {}

@Test
@ShardsFixed(num = 2)
public void test() throws Exception {
Expand Down Expand Up @@ -79,7 +75,7 @@ private void basicTest() throws Exception {
id_s1));

// The number of highlighted documents should be the same as the de-duplicated docs
assertEquals(1, rsp.getHighlighting().values().size());
assertEquals(1, rsp.getHighlighting().size());
}

private void randomizedTest() throws Exception {
Expand Down Expand Up @@ -142,7 +138,7 @@ private void randomizedTest() throws Exception {
"true"));
// The number of highlighted documents should be the same as the de-duplicated docs for this
// group
assertEquals(docsInGroup[group], rsp.getHighlighting().values().size());
assertEquals(docsInGroup[group], rsp.getHighlighting().size());
}
}

Expand Down
9 changes: 3 additions & 6 deletions solr/core/src/test/org/apache/solr/cli/PackageToolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,9 @@ public static void setupClusterWithSecurityEnabled() throws Exception {

@AfterClass
public static void teardown() throws Exception {
try {
if (repositoryServer != null) {
repositoryServer.stop();
}
} finally {
System.clearProperty("solr.packages.enabled");

if (repositoryServer != null) {
repositoryServer.stop();
}
}

Expand Down
2 changes: 0 additions & 2 deletions solr/core/src/test/org/apache/solr/cli/SolrProcessManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,6 @@ public static void afterClass() throws Exception {
processHttp.getValue().destroyForcibly();
processHttps.getValue().destroyForcibly();
SolrProcessManager.enableTestingMode = false;
System.clearProperty("solr.port.listen");
System.clearProperty("solr.pid.dir");
}

private static int findAvailablePort() throws IOException {
Expand Down
9 changes: 0 additions & 9 deletions solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
import org.apache.solr.embedded.JettySolrRunner;
import org.apache.solr.util.ExternalPaths;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import org.slf4j.Logger;
Expand All @@ -68,14 +67,6 @@ public static void beforeClass() {
System.setProperty("solr.directoryFactory", "solr.NRTCachingDirectoryFactory");
}

@AfterClass
public static void cleanup() {
System.clearProperty("solr.directoryFactory");
System.clearProperty("solr.host.advertise");
System.clearProperty("solr.port.listen");
System.clearProperty("solr.log.dir");
}

/**
* Overrides the call to exec bin/solr to start Solr nodes to start them using the Solr
* test-framework instead of the script, since the script depends on a full build.
Expand Down
55 changes: 19 additions & 36 deletions solr/core/src/test/org/apache/solr/cli/ZkSubcommandsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
import org.apache.solr.util.ExternalPaths;
import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.KeeperException;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import org.slf4j.Logger;
Expand Down Expand Up @@ -72,11 +71,6 @@ public static void beforeClass() {
System.setProperty("solrcloud.skip.autorecovery", "true");
}

@AfterClass
public static void afterClass() {
System.clearProperty("solrcloud.skip.autorecovery");
}

@Override
public void setUp() throws Exception {
super.setUp();
Expand Down Expand Up @@ -568,34 +562,25 @@ public void testSetClusterProperty() throws Exception {

@Test
public void testUpdateAcls() throws Exception {
try {
System.setProperty(
SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
DigestZkACLProvider.class.getName());
System.setProperty(
VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME, "user");
System.setProperty(
VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME, "pass");
System.setProperty(
SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
VMParamsAllAndReadonlyDigestZkACLProvider.class.getName());
System.setProperty(
VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME,
"user");
System.setProperty(
VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME,
"pass");

String[] args = new String[] {"updateacls", "/", "-z", zkServer.getZkAddress()};
assertEquals(0, CLITestHelper.runTool(args, UpdateACLTool.class));
} finally {
// Need to clear these before we open the next SolrZkClient
System.clearProperty(SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME);
System.clearProperty(
VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME);
System.clearProperty(
VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME);
}

System.setProperty(
SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME, DigestZkACLProvider.class.getName());
System.setProperty(
VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME, "user");
System.setProperty(
VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME, "pass");
System.setProperty(
SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
VMParamsAllAndReadonlyDigestZkACLProvider.class.getName());
System.setProperty(
VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME,
"user");
System.setProperty(
VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME,
"pass");

String[] args = new String[] {"updateacls", "/", "-z", zkServer.getZkAddress()};
assertEquals(0, CLITestHelper.runTool(args, UpdateACLTool.class));

Comment on lines +565 to 584
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of the try-finally block removes the protection against property leakage when updateacls tool execution fails. If the assertEquals assertion fails, the system properties will remain set, potentially affecting subsequent tests. The finally block was ensuring cleanup even when test assertions fail.

Copilot uses AI. Check for mistakes.
boolean excepted = false;
try (SolrZkClient zkClient =
Expand All @@ -619,8 +604,6 @@ public void tearDown() throws Exception {
if (zkServer != null) {
zkServer.shutdown();
}
System.clearProperty("solr.home");
System.clearProperty("minStateByteLenForCompression");
System.setOut(originalSystemOut);
super.tearDown();
}
Expand Down
1 change: 0 additions & 1 deletion .../core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServerSchemaAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ public static void destroyClass() throws IOException {
server.close();
server = null;
}
System.clearProperty("managed.schema.mutable");
}

@Before
Expand Down
1 change: 0 additions & 1 deletion solr/core/src/test/org/apache/solr/cloud/ChaosMonkeyNothingIsSafeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ public static void beforeSuperClass() {

@AfterClass
public static void afterSuperClass() {
System.clearProperty("solr.autoCommit.maxTime");
clearErrorHook();
}

Expand Down
1 change: 0 additions & 1 deletion solr/core/src/test/org/apache/solr/cloud/ChaosMonkeyNothingIsSafeWithPullReplicasTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ public static void beforeSuperClass() {

@AfterClass
public static void afterSuperClass() {
System.clearProperty("solr.autoCommit.maxTime");
clearErrorHook();
TestInjection.reset();
}
Expand Down
1 change: 0 additions & 1 deletion solr/core/src/test/org/apache/solr/cloud/ChaosMonkeySafeLeaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ public static void beforeSuperClass() {

@AfterClass
public static void afterSuperClass() {
System.clearProperty("solr.autoCommit.maxTime");
clearErrorHook();
}

Expand Down
1 change: 0 additions & 1 deletion solr/core/src/test/org/apache/solr/cloud/ChaosMonkeySafeLeaderWithPullReplicasTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ public static void beforeSuperClass() {

@AfterClass
public static void afterSuperClass() {
System.clearProperty("solr.autoCommit.maxTime");
clearErrorHook();
TestInjection.reset();
}
Expand Down
7 changes: 0 additions & 7 deletions solr/core/src/test/org/apache/solr/cloud/ClusterStateUpdateTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import org.apache.solr.common.cloud.Slice;
import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.embedded.JettySolrRunner;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;

Expand All @@ -37,12 +36,6 @@ public static void beforeClass() throws Exception {
configureCluster(3).addConfig("conf", configset("cloud-minimal")).configure();
}

@AfterClass
public static void afterClass() throws Exception {
shutdownCluster();
System.clearProperty("solrcloud.skip.autorecovery");
}

@Test
public void testCoreRegistration() throws Exception {
System.setProperty("solrcloud.update.delay", "1");
Expand Down
6 changes: 0 additions & 6 deletions solr/core/src/test/org/apache/solr/cloud/LeaderElectionIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import org.apache.solr.common.cloud.ZkNodeProps;
import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.embedded.JettySolrRunner;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;

Expand Down Expand Up @@ -153,9 +152,4 @@ private String getLeader(String collection) throws InterruptedException {

return props.getStr(ZkStateReader.NODE_NAME_PROP);
}

@AfterClass
public static void afterClass() {
System.clearProperty("solrcloud.skip.autorecovery");
}
}
Loading
Loading