Skip to content
Open
2 changes: 0 additions & 2 deletions solr/core/src/test/org/apache/solr/SolrTestCaseJ4Test.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ public static void beforeClass() throws Exception {
PathUtils.touch(tmpSolrHome.resolve("core0/core.properties"));
PathUtils.touch(tmpSolrHome.resolve("core1/core.properties"));

Files.copy(getFile("solr/solr.xml"), tmpSolrHome.resolve("solr.xml"));

initCore("solrconfig-minimal.xml", "schema-tiny.xml", tmpSolrHome, "core1");
}

Expand Down
17 changes: 5 additions & 12 deletions solr/core/src/test/org/apache/solr/TestCustomCoreProperties.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,11 @@ public static void beforeClass() throws Exception {

Files.createDirectories(confDir);

String src_dir = TEST_HOME() + "/collection1/conf";
Files.copy(Path.of(src_dir, "schema-tiny.xml"), confDir.resolve("schema.xml"));
String srcDir = TEST_HOME() + "/collection1/conf";
Files.copy(Path.of(srcDir, "schema-tiny.xml"), confDir.resolve("schema.xml"));
Files.copy(Path.of(srcDir, "solrconfig-coreproperties.xml"), confDir.resolve("solrconfig.xml"));
Files.copy(
Path.of(src_dir, "solrconfig-coreproperties.xml"), confDir.resolve("solrconfig.xml"));
Files.copy(
Path.of(src_dir, "solrconfig.snippet.randomindexconfig.xml"),
Path.of(srcDir, "solrconfig.snippet.randomindexconfig.xml"),
confDir.resolve("solrconfig.snippet.randomindexconfig.xml"));

Properties p = new Properties();
Expand All @@ -72,13 +71,7 @@ public static void beforeClass() throws Exception {
coreProperties.store(fos, null);
}

Properties nodeProperties = new Properties();
// this sets the property for jetty starting SolrDispatchFilter
if (System.getProperty("solr.data.dir") == null) {
nodeProperties.setProperty("solr.data.dir", createTempDir().toRealPath().toString());
}

solrTestRule.startSolr(homeDir, nodeProperties, JettyConfig.builder().build());
solrTestRule.startSolr(homeDir, new Properties(), JettyConfig.builder().build());
}

@Test
Expand Down
1 change: 0 additions & 1 deletion solr/core/src/test/org/apache/solr/core/TestLazyCores.java
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ public List<CoreDescriptor> discover(CoreContainer cc) {
private CoreContainer init() throws Exception {
solrHomeDirectory = createTempDir();

copyXmlToHome(solrHomeDirectory, "solr.xml");
for (int idx = 1; idx < 10; ++idx) {
copyMinConf(solrHomeDirectory.resolve("collection" + idx));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.ArrayList;
import java.util.Properties;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -62,14 +61,9 @@ public final class ReplicationTestHelper {
+ FileSystems.getDefault().getSeparator();

public static JettySolrRunner createAndStartJetty(SolrInstance instance) throws Exception {
Files.copy(
SolrTestCaseJ4.TEST_HOME().resolve("solr.xml"),
Path.of(instance.getHomeDir(), "solr.xml"),
StandardCopyOption.REPLACE_EXISTING);
Properties nodeProperties = new Properties();
nodeProperties.setProperty("solr.data.dir", instance.getDataDir());
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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".

JettyConfig jettyConfig = JettyConfig.builder().setPort(0).build();
JettySolrRunner jetty = new JettySolrRunner(instance.getHomeDir(), nodeProperties, jettyConfig);
JettySolrRunner jetty =
new JettySolrRunner(instance.getHomeDir(), new Properties(), jettyConfig);
jetty.start();
return jetty;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,9 @@ public class TestReplicationHandlerBackup extends SolrTestCaseJ4 {

private static JettySolrRunner createAndStartJetty(ReplicationTestHelper.SolrInstance instance)
throws Exception {
Files.copy(
SolrTestCaseJ4.TEST_HOME().resolve("solr.xml"), Path.of(instance.getHomeDir(), "solr.xml"));
Properties nodeProperties = new Properties();
nodeProperties.setProperty("solr.data.dir", instance.getDataDir());
JettyConfig jettyConfig = JettyConfig.builder().setPort(0).build();
JettySolrRunner jetty = new JettySolrRunner(instance.getHomeDir(), nodeProperties, jettyConfig);
JettySolrRunner jetty =
new JettySolrRunner(instance.getHomeDir(), new Properties(), jettyConfig);
jetty.start();
return jetty;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ public void setUp() throws Exception {
originalTestWait = IndexFetcher.testWait;

super.setUp();
// Disable URL allow-list checks for replication tests
System.setProperty("solr.security.allow.urls.enabled", "false");
System.setProperty("solr.directoryFactory", "solr.StandardDirectoryFactory");
String factory =
random().nextInt(100) < 75
Expand Down Expand Up @@ -114,6 +116,7 @@ public void tearDown() throws Exception {
followerClient = null;
}
System.clearProperty(TEST_URL_ALLOW_LIST);
System.clearProperty("solr.security.allow.urls.enabled");
Comment on lines 118 to +119
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, will do that.


IndexFetcher.usableDiskSpaceProvider = originalDiskSpaceprovider;
IndexFetcher.testWait = originalTestWait;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,9 @@ public class TestRestoreCore extends SolrTestCaseJ4 {

private static JettySolrRunner createAndStartJetty(ReplicationTestHelper.SolrInstance instance)
throws Exception {
Files.copy(
SolrTestCaseJ4.TEST_HOME().resolve("solr.xml"), Path.of(instance.getHomeDir(), "solr.xml"));
Properties nodeProperties = new Properties();
nodeProperties.setProperty("solr.data.dir", instance.getDataDir());
JettyConfig jettyConfig = JettyConfig.builder().setPort(0).build();
JettySolrRunner jetty = new JettySolrRunner(instance.getHomeDir(), nodeProperties, jettyConfig);
JettySolrRunner jetty =
new JettySolrRunner(instance.getHomeDir(), new Properties(), jettyConfig);
jetty.start();
return jetty;
}
Expand Down Expand Up @@ -179,7 +176,7 @@ public void testSimpleRestore() throws Exception {
}
}

public void testBackupFailsMissingAllowPaths() throws Exception {
public void testBackupFailsMissingAllowPaths() {
final String params =
"&location=" + URLEncoder.encode(createTempDir().toString(), StandardCharsets.UTF_8);
Throwable t =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

package org.apache.solr.handler;

import java.nio.file.Files;
import java.nio.file.Path;
import org.apache.commons.io.file.PathUtils;
import org.apache.solr.SolrTestCaseJ4;
Expand All @@ -35,7 +34,6 @@ public void testWelcomeMessage() throws Exception {
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"));
Copy link
Contributor

Choose a reason for hiding this comment

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

So why is solr.xml copying no longer necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


JettySolrRunner jetty =
new JettySolrRunner(solrHomeTmp.toString(), JettyConfig.builder().build());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@
*/
public class JerseyApplicationSharingTest extends SolrCloudTestCase {

private static final String collection = "collection1";
private static final String confDir = collection + "/conf";

@BeforeClass
public static void setupCluster() throws Exception {
configureCluster(1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import java.util.Map;
import java.util.Properties;
import java.util.regex.Pattern;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer;
import org.apache.solr.client.solrj.impl.CloudSolrClient;
Expand Down Expand Up @@ -80,7 +79,6 @@ private static void initStandalone() throws Exception {
final Path collDir = homeDir.resolve("collection1");
final Path confDir = collDir.resolve("conf");
Files.createDirectories(confDir);
Files.copy(SolrTestCaseJ4.TEST_HOME().resolve("solr.xml"), homeDir.resolve("solr.xml"));
String src_dir = TEST_HOME() + "/collection1/conf";
Files.copy(Path.of(src_dir, "schema_latest.xml"), confDir.resolve("schema.xml"));
Files.copy(Path.of(src_dir, "solrconfig-minimal.xml"), confDir.resolve("solrconfig.xml"));
Expand All @@ -96,11 +94,9 @@ private static void initStandalone() throws Exception {
Files.copy(Path.of(src_dir, file), confDir.resolve(file));
}
Files.createFile(collDir.resolve("core.properties"));
Properties nodeProperties = new Properties();
nodeProperties.setProperty("solr.data.dir", h.getCore().getDataDir());
JSR =
new JettySolrRunner(
homeDir.toAbsolutePath().toString(), nodeProperties, JettyConfig.builder().build());
homeDir.toAbsolutePath().toString(), new Properties(), JettyConfig.builder().build());
}

private static void initCloud() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public void testSimple() throws Exception {
assertEquals(3, res.size());
assertEquals(3, beans.size());
for (SolrDocument d : res) {
Integer id = Integer.parseInt(d.getFieldValue("id").toString());
int id = Integer.parseInt(d.getFieldValue("id").toString());
for (String field : new String[] {"data", "data_dv"}) {
byte[] data = (byte[]) d.getFieldValue(field);
if (id == 1) {
Expand All @@ -116,7 +116,7 @@ public void testSimple() throws Exception {
}
}
for (Bean d : beans) {
Integer id = Integer.parseInt(d.id);
int id = Integer.parseInt(d.id);
for (byte[] data : new byte[][] {d.data, d.data_dv}) {
if (id == 1) {
assertEquals(5, data.length);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Base64;
import java.util.Collections;
Expand Down Expand Up @@ -53,9 +52,6 @@
public class BasicAuthStandaloneTest extends SolrTestCaseJ4 {

private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private static final Path ROOT_DIR = TEST_HOME();
private static final Path CONF_DIR =
ROOT_DIR.resolve("configsets").resolve("configset-2").resolve("conf");
Comment on lines -57 to -58
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


SecurityConfHandlerLocalForTesting securityConfHandler;
SolrInstance instance = null;
Expand Down Expand Up @@ -198,11 +194,9 @@ public static void setBasicAuthHeader(AbstractHttpMessage httpMsg, String user,
}

static JettySolrRunner createAndStartJetty(SolrInstance instance) throws Exception {
Properties nodeProperties = new Properties();
nodeProperties.setProperty("solr.data.dir", instance.getDataDir().toString());
JettySolrRunner jetty =
new JettySolrRunner(
instance.getHomeDir().toString(), nodeProperties, JettyConfig.builder().build());
instance.getHomeDir().toString(), new Properties(), JettyConfig.builder().build());
jetty.start();
return jetty;
}
Expand All @@ -211,7 +205,6 @@ static class SolrInstance {
String name;
Integer port;
Path homeDir;
Path confDir;
Path dataDir;

/**
Expand All @@ -227,36 +220,13 @@ public Path getHomeDir() {
return homeDir;
}

public Path getSchemaFile() {
return CONF_DIR.resolve("schema.xml");
}

public Path getDataDir() {
return dataDir;
}

public Path getSolrConfigFile() {
return CONF_DIR.resolve("solrconfig.xml");
}

public Path getSolrXmlFile() {
return ROOT_DIR.resolve("solr.xml");
}

public void setUp() throws Exception {
homeDir = createTempDir(name).toAbsolutePath();
dataDir = homeDir.resolve("collection1").resolve("data");
confDir = homeDir.resolve("collection1").resolve("conf");

Files.createDirectories(homeDir);
Files.createDirectories(dataDir);
Files.createDirectories(confDir);

Files.copy(getSolrXmlFile(), homeDir.resolve("solr.xml"));
Files.copy(getSolrConfigFile(), confDir.resolve("solrconfig.xml"));
Files.copy(getSchemaFile(), confDir.resolve("schema.xml"));

Files.createFile(homeDir.resolve("collection1").resolve("core.properties"));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -278,10 +278,6 @@ public String getHomeDir() {
return homeDir.toString();
}

public String getUrl() {
return buildUrl(port) + "/collection1";
}

public String getBaseUrl() {
return buildUrl(port);
}
Expand All @@ -294,10 +290,6 @@ public String getSchemaFile() {
return "solrj/solr/collection1/conf/schema-replication1.xml";
}

public String getConfDir() {
return confDir.toString();
}

public String getDataDir() {
return dataDir.toString();
}
Expand All @@ -306,17 +298,11 @@ public String getSolrConfigFile() {
return "solrj/solr/collection1/conf/solrconfig-follower1.xml";
}

public String getSolrXmlFile() {
return "solrj/solr/solr.xml";
}

public void setUp() throws Exception {
Files.createDirectories(homeDir);
Files.createDirectories(dataDir);
Files.createDirectories(confDir);

Files.copy(SolrTestCaseJ4.getFile(getSolrXmlFile()), homeDir.resolve("solr.xml"));

Path f = confDir.resolve("solrconfig.xml");
Files.copy(SolrTestCaseJ4.getFile(getSolrConfigFile()), f);
f = confDir.resolve("schema.xml");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,10 +256,6 @@ public String getHomeDir() {
return homeDir.toString();
}

public String getUrl() {
return buildUrl(port) + "/collection1";
}

public String getBaseUrl() {
return buildUrl(port);
}
Expand All @@ -272,10 +268,6 @@ public String getSchemaFile() {
return "solrj/solr/collection1/conf/schema-replication1.xml";
}

public String getConfDir() {
return confDir.toString();
}

public String getDataDir() {
return dataDir.toString();
}
Expand All @@ -284,17 +276,11 @@ public String getSolrConfigFile() {
return "solrj/solr/collection1/conf/solrconfig-follower1.xml";
}

public String getSolrXmlFile() {
return "solrj/solr/solr.xml";
}

public void setUp() throws Exception {
Files.createDirectories(homeDir);
Files.createDirectories(dataDir);
Files.createDirectories(confDir);

Files.copy(SolrTestCaseJ4.getFile(getSolrXmlFile()), homeDir.resolve("solr.xml"));

Path f = confDir.resolve("solrconfig.xml");
Files.copy(SolrTestCaseJ4.getFile(getSolrConfigFile()), f);
f = confDir.resolve("schema.xml");
Expand Down
14 changes: 0 additions & 14 deletions solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
Original file line number Diff line number Diff line change
Expand Up @@ -2258,19 +2258,6 @@ public static void copyMinConf(Path dstRoot, String propertiesContent, String so
subHome.resolve("solrconfig.snippet.randomindexconfig.xml"));
}

// Creates minimal full setup, including solr.xml
public static void copyMinFullSetup(Path dstRoot) throws IOException {
Files.createDirectories(dstRoot);
Files.copy(SolrTestCaseJ4.TEST_PATH().resolve("solr.xml"), dstRoot.resolve("solr.xml"));
copyMinConf(dstRoot);
}

// Just copies the file indicated to the tmp home directory naming it "solr.xml"
public static void copyXmlToHome(Path dstRoot, String fromFile) throws IOException {
Files.createDirectories(dstRoot);
Files.copy(SolrTestCaseJ4.TEST_PATH().resolve(fromFile), dstRoot.resolve("solr.xml"));
}

// Creates a consistent configuration, _including_ solr.xml at dstRoot. Creates collection1/conf
// and copies the stock files in there.

Expand Down Expand Up @@ -2314,7 +2301,6 @@ public static Path legacyExampleCollection1SolrHome() {
try {
Path tempSolrHome = FilterPath.unwrap(LuceneTestCase.createTempDir());
Path serverSolr = tempSolrHome.resolve(sourceHome).resolve("server").resolve("solr");
Files.copy(serverSolr.resolve("solr.xml"), tempSolrHome.resolve("solr.xml"));

Path sourceConfig = serverSolr.resolve("configsets").resolve("sample_techproducts_configs");
Path collection1Dir = tempSolrHome.resolve("collection1");
Expand Down