Copilot commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2626624629
##########
solr/test-framework/src/java/org/apache/solr/BaseDistributedSearchTestCase.java:
##########
@@ -480,10 +480,10 @@ private SolrClient clientFor(SolrInputDocument doc) {
/** Indexes the document in both the control client, and a randomly selected
client */
protected void indexDoc(SolrInputDocument doc) throws IOException,
SolrServerException {
- indexDoc(clientFor(doc), null, doc);
+ indexDoc(clientFor(doc), doc);
}
- protected void indexDoc(SolrClient client, SolrParams params,
SolrInputDocument doc)
+ protected void indexDoc(SolrClient client, SolrInputDocument doc)
Review Comment:
The method signature changed from "indexDoc(SolrClient client, SolrParams
params, SolrInputDocument doc)" to "indexDoc(SolrClient client,
SolrInputDocument doc)", removing the SolrParams parameter. This is a protected
method in a base test class, so any subclass that was calling this method with
three parameters will break. Looking at the usage in
BasicDistributedZkTest.java (line 210), the params parameter was being used to
pass commit=true. While this specific usage may not have been doing anything
meaningful, removing the parameter entirely is a breaking API change that could
affect other tests not visible in this PR.
##########
solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java:
##########
@@ -195,7 +195,7 @@ public void testCanDeleteEmptyOrFullDirectories() throws
Exception {
}
@Test
- public void testDirectoryCreationFailsIfParentDoesntExist() throws Exception
{
+ public void testDirectoryCreationFailsIfParentDoesNotExist() throws
Exception {
Review Comment:
The method "testDirectoryCreationFailsIfParentDoesntExist" was renamed to
"testDirectoryCreationFailsIfParentDoesNotExist". This is a test method that
may be referenced in documentation or CI/CD pipelines. While the new name is
grammatically correct, test method renames can cause issues with test filtering
or reporting tools that may reference the old name.
##########
solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractInstallShardTest.java:
##########
@@ -382,8 +382,7 @@ private static void enableReadOnly(String collectionName)
throws Exception {
.process(cluster.getSolrClient());
}
- private void runParallelShardInstalls(String collectionName, URI[]
dataLocations)
- throws Exception {
+ private void runParallelShardInstalls(String collectionName) throws
Exception {
Review Comment:
The method signature changed from "runParallelShardInstalls(String
collectionName, URI[] dataLocations)" to "runParallelShardInstalls(String
collectionName)". The dataLocations parameter was removed, but the method now
references the field "multiShardUris" directly. While this works, if any
subclass was overriding this method and passing different URIs, that
functionality would be broken. Consider keeping the parameter for extensibility
or making the method private if it's not intended to be overridden.
##########
solr/test-framework/src/java/org/apache/solr/util/NotSecurePseudoRandomSpi.java:
##########
@@ -42,47 +41,19 @@ protected byte[] engineGenerateSeed(int numBytes) {
}
/**
- * Helper method that can be used to fill an array with non-zero data.
(Attempted workarround of
+ * Helper method that can be used to fill an array with non-zero data.
(Attempted workaround of
* Solaris SSL Padding bug: SOLR-9068)
*/
- private static final byte[] fillData(byte[] data) {
+ private static void fillData(byte[] data) {
ThreadLocalRandom.current().nextBytes(data);
- return data;
}
Review Comment:
The return type of fillData changed from "byte[]" to "void". The previous
implementation returned the data array after filling it, but the new version
doesn't return anything. Any code that was chaining this method call (e.g.,
using the return value) would break. Since this is a private static method, it
should be safe, but verify that all call sites only use this method for its
side effect and don't rely on the return value.
##########
solr/test-framework/src/java/org/apache/solr/util/SSLTestConfig.java:
##########
@@ -330,16 +330,16 @@ public Registry<ConnectionSocketFactory>
getSocketFactoryRegistry() {
/**
* A mocked up instance of SecureRandom that just uses {@link Random} under
the covers. This is to
* prevent blocking issues that arise in platform default SecureRandom
instances due to too many
- * instances / not enough random entropy. Tests do not need secure SSL.
+ * instances / not enough random entropy. Tests do not need to use secure
SSL.
*/
private static class NotSecurePseudoRandom extends SecureRandom {
public static final SecureRandom INSTANCE = new NotSecurePseudoRandom();
/**
- * Helper method that can be used to fill an array with non-zero data.
(Attempted workarround of
+ * Helper method that can be used to fill an array with non-zero data.
(Attempted workaround of
* Solaris SSL Padding bug: SOLR-9068)
*/
- private static final byte[] fillData(byte[] data) {
+ private static byte[] fillData(byte[] data) {
ThreadLocalRandom.current().nextBytes(data);
return data;
}
Review Comment:
The fillData method visibility changed from "private static final" to
"private static". While removing "final" is fine for static methods (it doesn't
affect behavior), the return type also changed from "byte[]" to "void", and the
"return data;" statement was removed. This changes the API of the method.
Verify that the SSLTestConfig class (where this method is also defined) is
updated consistently and that no code relies on the return value.
##########
solr/test-framework/src/java/org/apache/solr/BaseDistributedSearchTestCase.java:
##########
@@ -808,17 +807,15 @@ public static String compare1(
public static String compare(
@SuppressWarnings({"rawtypes"}) Map a,
@SuppressWarnings({"rawtypes"}) Map b,
- int flags,
Map<String, Integer> handle) {
String cmp;
- cmp = compare1(a, b, flags, handle);
+ cmp = compare1(a, b, handle);
if (cmp != null) return cmp;
- return compare1(b, a, flags, handle);
+ return compare1(b, a, handle);
}
- public static String compare(
- SolrDocument a, SolrDocument b, int flags, Map<String, Integer> handle) {
- return compare(a.getFieldValuesMap(), b.getFieldValuesMap(), flags,
handle);
+ public static String compare(SolrDocument a, SolrDocument b, Map<String,
Integer> handle) {
+ return compare(a.getFieldValuesMap(), b.getFieldValuesMap(), handle);
Review Comment:
The compare methods had their signatures changed to remove the "flags"
parameter (which was being passed as 0 in all cases). While this simplifies the
API, this is a public static method in a base test class. If any external code
or tests were calling these compare methods with the flags parameter, those
calls will break. Consider deprecating the old methods first rather than
removing the parameter entirely.
##########
solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractInstallShardTest.java:
##########
@@ -82,7 +82,7 @@ public static void seedDocGenerator() {
}
@Before
- public void clearCollsToDelete() {
+ public void clearCollectionsToDelete() {
Review Comment:
The method name was changed from "clearCollsToDelete" to
"clearCollectionsToDelete". However, this is a public method (used in setUp),
so changing its name could break tests that override this method in subclasses.
This change should be reverted to maintain backward compatibility, or the
method should be marked as private if it's only used internally.
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractZkTestCase.java:
##########
@@ -65,7 +65,7 @@ public static void azt_beforeClass() throws Exception {
}
@AfterClass
- public static void azt_afterClass() throws Exception {
+ public static void aztAfterClass() throws Exception {
Review Comment:
The method "azt_afterClass" was renamed to "aztAfterClass". This changes the
naming convention used in this test class. The original naming used underscores
to separate "azt" (likely meaning "AbstractZkTestCase") from "afterClass". The
new name removes the underscore, which is inconsistent with the other method
"azt_beforeClass" that still uses an underscore. Both methods should follow the
same naming convention.
--
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]
