-
Notifications
You must be signed in to change notification settings - Fork 794
Remove extra solr.xml and other config setup for tests that are not needed #3988
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
ee7ebed
4079397
91d2f92
e8b8bca
e10ce96
427b35e
150995b
21b14cc
8330f9c
14ccb3a
f14e67f
f8d6c93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clearing properties in an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good suggestion, will do that. |
||
|
|
||
| IndexFetcher.usableDiskSpaceProvider = originalDiskSpaceprovider; | ||
| IndexFetcher.testWait = originalTestWait; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So why is solr.xml copying no longer necessary?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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!).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that I documented the rule's |
||
|
|
||
| JettySolrRunner jetty = | ||
| new JettySolrRunner(solrHomeTmp.toString(), JettyConfig.builder().build()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets not change this in this PR. |
||
|
|
||
| SecurityConfHandlerLocalForTesting securityConfHandler; | ||
| SolrInstance instance = null; | ||
|
|
@@ -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; | ||
| } | ||
|
|
@@ -211,7 +205,6 @@ static class SolrInstance { | |
| String name; | ||
| Integer port; | ||
| Path homeDir; | ||
| Path confDir; | ||
| Path dataDir; | ||
|
|
||
| /** | ||
|
|
@@ -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")); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.dirand did the same thing...There was a problem hiding this comment.
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".