Copilot commented on code in PR #3947:
URL: https://github.com/apache/solr/pull/3947#discussion_r2643735303
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTestsBase.java:
##########
@@ -49,24 +56,37 @@ public void after() {
}
}
client = null;
+ super.tearDown();
}
- @Override
- public SolrClient getSolrClient() {
+ protected SolrClient getSolrClient() {
if (client == null) {
client = createNewSolrClient();
}
return client;
}
/**
- * Create a new solr client. If createJetty was called, a http
implementation will be created,
+ * Create a new solr client. If createJetty was called, an http
implementation will be created,
* otherwise an embedded implementation will be created. Subclasses should
override for other
* options.
*/
- @Override
public SolrClient createNewSolrClient() {
- return getHttpSolrClient(getBaseUrl(), DEFAULT_TEST_CORENAME);
+ return SolrTestCaseJ4.getHttpSolrClient(solrJettyTestRule.getBaseUrl(),
DEFAULT_TEST_CORENAME);
+ }
Review Comment:
The `createNewSolrClient` method has had its `@Override` annotation removed.
This suggests it was previously overriding a method from the parent class
(`SolrJettyTestBase`), but now with the parent changed to `SolrTestCaseJ4`, it
no longer overrides anything. Verify this change is intentional and doesn't
break the expected polymorphic behavior in subclasses.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTestsBase.java:
##########
@@ -49,24 +56,37 @@ public void after() {
}
}
client = null;
+ super.tearDown();
}
- @Override
- public SolrClient getSolrClient() {
+ protected SolrClient getSolrClient() {
if (client == null) {
client = createNewSolrClient();
}
return client;
}
/**
- * Create a new solr client. If createJetty was called, a http
implementation will be created,
+ * Create a new solr client. If createJetty was called, an http
implementation will be created,
* otherwise an embedded implementation will be created. Subclasses should
override for other
* options.
*/
- @Override
public SolrClient createNewSolrClient() {
- return getHttpSolrClient(getBaseUrl(), DEFAULT_TEST_CORENAME);
+ return SolrTestCaseJ4.getHttpSolrClient(solrJettyTestRule.getBaseUrl(),
DEFAULT_TEST_CORENAME);
+ }
+
+ protected static String getCoreUrl() {
+ return solrJettyTestRule.getBaseUrl() + "/" + DEFAULT_TEST_CORENAME;
+ }
+
+ protected static String getBaseUrl() {
+ return solrJettyTestRule.getBaseUrl();
+ }
+
+ // Backward compatibility methods for existing subclasses
+ @Deprecated
+ protected static void createAndStartJetty(Path solrHome) throws Exception {
+ solrJettyTestRule.startSolr(solrHome, new Properties(),
JettyConfig.builder().build());
}
Review Comment:
The deprecated method `createAndStartJetty` is being added to
`SolrExampleTestsBase` but marked as deprecated. This suggests subclasses may
still be calling it. Consider removing this method entirely and updating all
subclasses to use the new pattern with `solrJettyTestRule.startSolr()` instead
of maintaining backward compatibility with a deprecated API.
```suggestion
```
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java:
##########
@@ -58,12 +59,16 @@
import org.apache.solr.util.ServletFixtures.RedirectServlet;
import org.apache.solr.util.ServletFixtures.SlowServlet;
import org.apache.solr.util.ServletFixtures.SlowStreamServlet;
+import org.apache.solr.util.SolrJettyTestRule;
import org.eclipse.jetty.ee10.servlet.ServletHolder;
import org.junit.BeforeClass;
+import org.junit.ClassRule;
-public abstract class HttpSolrClientTestBase extends SolrJettyTestBase {
+public abstract class HttpSolrClientTestBase extends SolrTestCaseJ4 {
- protected static final String DEFAULT_CORE = "foo";
+ @ClassRule public static SolrJettyTestRule solrClientTestRule = new
SolrJettyTestRule();
+
+ protected static final String DEFAULT_COLLECTION = "collection1";
Review Comment:
The constant `DEFAULT_CORE` has been renamed to `DEFAULT_COLLECTION`.
However, this constant is being used throughout the test methods which may
indicate an incomplete migration. Consider verifying that all references to the
old constant name have been updated consistently. Additionally,
`DEFAULT_COLLECTION` should likely use the same value as
`DEFAULT_TEST_CORENAME` for consistency across the codebase.
```suggestion
protected static final String DEFAULT_COLLECTION = DEFAULT_TEST_CORENAME;
```
##########
solr/core/src/test/org/apache/solr/handler/component/DistributedDebugComponentTest.java:
##########
@@ -93,14 +98,8 @@ public static void createThings() throws Exception {
@AfterClass
public static void destroyThings() throws Exception {
- if (null != collection1) {
- collection1.close();
- collection1 = null;
- }
- if (null != collection2) {
- collection2.close();
- collection2 = null;
- }
+ collection1 = null;
+ collection2 = null;
Review Comment:
In the migration to SolrJettyTestRule, the clients `collection1` and
`collection2` are no longer explicitly closed in the AfterClass method. While
the comment suggests relying on the rule for cleanup, verify that the
SolrJettyTestRule properly closes all SolrClient instances obtained via
`getSolrClient()` to prevent resource leaks.
```suggestion
if (collection1 != null) {
collection1.close();
collection1 = null;
}
if (collection2 != null) {
collection2.close();
collection2 = null;
}
```
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrSchemalessExampleTest.java:
##########
@@ -42,6 +42,11 @@
public class SolrSchemalessExampleTest extends SolrExampleTestsBase {
+ protected HttpClient getHttpClient() {
+ HttpSolrClient client = (HttpSolrClient) getSolrClient();
+ return client.getHttpClient();
+ }
Review Comment:
The method `getHttpClient()` is added but is never called within this file.
If this is an override of a parent class method or an interface method, it
should be annotated with `@Override`. Otherwise, consider removing this unused
method or documenting its purpose.
##########
solr/core/src/test/org/apache/solr/TestTolerantSearch.java:
##########
@@ -100,14 +114,8 @@ public static void createThings() throws Exception {
@AfterClass
public static void destroyThings() throws Exception {
- if (null != collection1) {
- collection1.close();
- collection1 = null;
- }
- if (null != collection2) {
- collection2.close();
- collection2 = null;
- }
+ collection1 = null;
+ collection2 = null;
Review Comment:
The clients `collection1` and `collection2` in TestTolerantSearch are set to
null in AfterClass without being closed. This creates a potential resource
leak. Either close these clients before nulling them, or verify that the
SolrJettyTestRule handles cleanup of all client instances.
```suggestion
if (collection1 != null) {
collection1.close();
collection1 = null;
}
if (collection2 != null) {
collection2.close();
collection2 = null;
}
```
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTestsBase.java:
##########
@@ -49,24 +56,37 @@ public void after() {
}
}
client = null;
+ super.tearDown();
}
- @Override
- public SolrClient getSolrClient() {
+ protected SolrClient getSolrClient() {
if (client == null) {
client = createNewSolrClient();
}
return client;
}
Review Comment:
The `getSolrClient` method visibility has changed from `public` (with
`@Override`) to `protected` (without `@Override`). This is a breaking change
for any code that was relying on this being a public method. If this method is
part of a base class contract, verify that all subclasses and callers can still
access it appropriately.
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandlerNonCloud.java:
##########
@@ -20,35 +20,44 @@
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
-import org.apache.solr.SolrJettyTestBase;
+import java.util.Properties;
+import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.solrj.io.Tuple;
import org.apache.solr.client.solrj.io.stream.SolrStream;
import org.apache.solr.client.solrj.io.stream.TupleStream;
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.IOUtils;
+import org.apache.solr.embedded.JettyConfig;
+import org.apache.solr.util.SolrJettyTestRule;
import org.junit.BeforeClass;
+import org.junit.ClassRule;
import org.junit.Test;
-public class TestSQLHandlerNonCloud extends SolrJettyTestBase {
+public class TestSQLHandlerNonCloud extends SolrTestCaseJ4 {
+
+ @ClassRule public static SolrJettyTestRule solrJettyTestRule = new
SolrJettyTestRule();
private static Path createSolrHome() throws Exception {
- Path workDir = createTempDir();
- setupJettyTestHome(workDir, DEFAULT_TEST_COLLECTION_NAME);
+ Path workDir = createTempDir().toRealPath();
+ Path collectionDirectory = workDir.resolve(DEFAULT_TEST_COLLECTION_NAME);
+
+ copyMinConf(collectionDirectory, "name=" + DEFAULT_TEST_COLLECTION_NAME +
"\n");
+
return workDir;
Review Comment:
The migration removes the `setupJettyTestHome` call and replaces it with
`copyMinConf`. However, `setupJettyTestHome` may have been copying additional
configuration files beyond what `copyMinConf` provides. Verify that all
necessary configuration files (such as schema.xml, managed-schema, etc.) are
still properly copied for the test to function correctly.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java:
##########
@@ -87,18 +87,14 @@
/**
* This should include tests against the example solr config
*
- * <p>This lets us try various SolrServer implementations with the same tests.
+ * <p>This lets us try various SolrClient implementations with the same tests.
*
* @since solr 1.3
*/
@SuppressSSL
public abstract class SolrExampleTests extends SolrExampleTestsBase {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
The static block that calls `ignoreException("uniqueKey")` has been removed.
If this exception ignore was serving a purpose in handling expected exceptions
during tests, its removal could cause test failures or unexpected error
logging. Verify that this removal is intentional and doesn't affect test
behavior.
```suggestion
static {
ignoreException("uniqueKey");
}
```
##########
solr/core/src/test/org/apache/solr/TestTolerantSearch.java:
##########
@@ -31,52 +31,66 @@
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.params.ShardParams;
import org.apache.solr.common.util.NamedList;
+import org.apache.solr.embedded.JettyConfig;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.response.JavaBinResponseWriter;
import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJettyTestRule;
import org.junit.AfterClass;
import org.junit.BeforeClass;
+import org.junit.ClassRule;
-public class TestTolerantSearch extends SolrJettyTestBase {
+public class TestTolerantSearch extends SolrTestCaseJ4 {
+
+ @ClassRule public static SolrJettyTestRule solrJettyTestRule = new
SolrJettyTestRule();
private static SolrClient collection1;
private static SolrClient collection2;
private static String shard1;
private static String shard2;
private static Path createSolrHome() throws Exception {
- Path workDir = createTempDir();
- setupJettyTestHome(workDir, "collection1");
+ Path workDir = createTempDir().toRealPath();
+
+ // Copy solr.xml
Files.copy(
- Path.of(SolrTestCaseJ4.TEST_HOME() +
"/collection1/conf/solrconfig-tolerant-search.xml"),
-
workDir.resolve("collection1").resolve("conf").resolve("solrconfig.xml"),
+ SolrTestCaseJ4.TEST_PATH().resolve("solr.xml"),
+ workDir.resolve("solr.xml"),
StandardCopyOption.REPLACE_EXISTING);
- FileUtils.copyDirectory(
- workDir.resolve("collection1").toFile(),
workDir.resolve("collection2").toFile());
+
+ // Set up collection1 with minimal config + tolerant search solrconfig
+ Path collection1Dir = workDir.resolve("collection1");
+ copyMinConf(collection1Dir, "name=collection1\n",
"solrconfig-tolerant-search.xml");
+
+ // Set up configset for CoreAdminRequest.Create (reuse the same config)
+ Path configSetDir = workDir.resolve("configsets").resolve("collection1");
+ copyMinConf(configSetDir, null, "solrconfig-tolerant-search.xml");
+
return workDir;
Review Comment:
Similar issue in TestTolerantSearch - the migration replaces
`setupJettyTestHome` and `FileUtils.copyDirectory` with `copyMinConf`, but
doesn't verify that all necessary configuration files are copied. The original
setup copied an entire collection directory, while the new approach only
creates minimal config. This could lead to missing configuration files required
for the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
