Re: [PR] Test framework code review and clean ups [solr]
epugh merged PR #3920: URL: https://github.com/apache/solr/pull/3920 -- 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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on PR #3920: URL: https://github.com/apache/solr/pull/3920#issuecomment-3670142323 @dsmiley I am merging to main and branch_10_x, but NOT branch_10_0. Thanks for the review on this. -- 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]
Re: [PR] Test framework code review and clean ups [solr]
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
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-z
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on PR #3920: URL: https://github.com/apache/solr/pull/3920#issuecomment-3664816479 @dsmiley I went through your comments and followed them. I think this is now ready. Thanks for going through a rather LONG PR. In retrospect, I wish I had separated the typo fixing from the actual code changes, as that would have made review easier. -- 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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920: URL: https://github.com/apache/solr/pull/3920#discussion_r2626587658 ## solr/test-framework/src/java/org/apache/solr/util/SolrMetricTestUtils.java: ## @@ -207,17 +196,6 @@ public static CounterSnapshot.CounterDataPointSnapshot newCloudSelectRequestsDat .build()); } - public static CounterSnapshot.CounterDataPointSnapshot newStandaloneUpdateRequestsDatapoint( Review Comment: done -- 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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2626581317
##
solr/test-framework/src/java/org/apache/solr/util/SolrClientTestRule.java:
##
@@ -85,11 +84,6 @@ public NewCollectionBuilder withConfigSet(String configSet) {
return this;
}
-public NewCollectionBuilder withConfigFile(String configFile) {
Review Comment:
I'm going to back the removed code out in the interest of moving forward. I
do think it's interesting that none of the existing tests provide a custom
solrconfig.xml. HOwever, maybe when we finish migrating to using
`SolrClientTestRule` the use or non-use of solrconfig.xml will become more
apparent!
--
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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2626568849
##
solr/test-framework/src/java/org/apache/solr/util/SSLTestConfig.java:
##
@@ -61,11 +61,6 @@ public class SSLTestConfig {
private final boolean useSsl;
private final boolean clientAuth;
- /** Creates an SSLTestConfig that does not use SSL or client authentication
*/
- public SSLTestConfig() {
Review Comment:
done
--
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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2626557720
##
solr/test-framework/src/java/org/apache/solr/search/facet/DebugAgg.java:
##
@@ -38,14 +38,12 @@ public ValueSource parse(FunctionQParser fp) throws
SyntaxError {
parses.incrementAndGet();
final String what = fp.hasMoreArguments() ? fp.parseId() : "wrap";
- switch (what) {
-case "wrap":
- return new DebugAgg(fp);
-case "numShards":
- return new DebugAggNumShards();
-default: /* No-Op */
- }
- throw new RuntimeException("No idea what to do with " + what);
+ /* No-Op */
+ return switch (what) {
Review Comment:
;-). switch I like!
--
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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2626555882
##
solr/test-framework/src/java/org/apache/solr/common/cloud/ClusterStateUtil.java:
##
@@ -31,18 +31,6 @@ public class ClusterStateUtil {
private static final int TIMEOUT_POLL_MS = 1000;
- /**
- * Wait to see *all* cores live and active.
- *
- * @param zkStateReader to use for ClusterState
- * @param timeoutInMs how long to wait before giving up
- * @return false if timed out
- */
- public static boolean waitForAllActiveAndLiveReplicas(
Review Comment:
done
--
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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2626551484
##
solr/test-framework/src/java/org/apache/solr/cluster/placement/Builders.java:
##
@@ -501,10 +493,6 @@ public String getShardName() {
return shardName;
}
-public List getReplicaBuilders() {
Review Comment:
Done!
--
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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2626534964
##
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 {
Review Comment:
Although, if the tests are passing without it, do we even need this method?
--
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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2626532389
##
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 {
Review Comment:
I think I understand, you could imagine that there is more than one
`@AfterClass`, so this is to prevent it from being overriding...
--
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]
Re: [PR] Test framework code review and clean ups [solr]
dsmiley commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2589256176
##
solr/test-framework/src/java/org/apache/solr/util/SolrClientTestRule.java:
##
@@ -109,10 +103,6 @@ public String getConfigSet() {
return configSet;
}
-public String getConfigFile() {
Review Comment:
basically, don't remove code from this whole test rule
##
solr/test-framework/src/java/org/apache/solr/util/SolrMetricTestUtils.java:
##
@@ -207,17 +196,6 @@ public static CounterSnapshot.CounterDataPointSnapshot
newCloudSelectRequestsDat
.build());
}
- public static CounterSnapshot.CounterDataPointSnapshot
newStandaloneUpdateRequestsDatapoint(
Review Comment:
keep
##
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 {
Review Comment:
minor: probably has your attention because of the underscore but you could
simply change to `aztAfterClass`. The author likely wanted to disambiguate on
a common base class -- reasonable.
##
solr/test-framework/src/java/org/apache/solr/search/facet/DebugAgg.java:
##
@@ -38,14 +38,12 @@ public ValueSource parse(FunctionQParser fp) throws
SyntaxError {
parses.incrementAndGet();
final String what = fp.hasMoreArguments() ? fp.parseId() : "wrap";
- switch (what) {
-case "wrap":
- return new DebugAgg(fp);
-case "numShards":
- return new DebugAggNumShards();
-default: /* No-Op */
- }
- throw new RuntimeException("No idea what to do with " + what);
+ /* No-Op */
+ return switch (what) {
Review Comment:
I see you took a moment of pleasure to do Java 21 stuff :-)
##
solr/test-framework/src/java/org/apache/solr/util/SSLTestConfig.java:
##
@@ -61,11 +61,6 @@ public class SSLTestConfig {
private final boolean useSsl;
private final boolean clientAuth;
- /** Creates an SSLTestConfig that does not use SSL or client authentication
*/
- public SSLTestConfig() {
Review Comment:
keep
##
solr/test-framework/src/java/org/apache/solr/cluster/placement/Builders.java:
##
@@ -501,10 +493,6 @@ public String getShardName() {
return shardName;
}
-public List getReplicaBuilders() {
Review Comment:
I suggest keeping the methods you removed in this file
##
solr/test-framework/src/java/org/apache/solr/common/cloud/ClusterStateUtil.java:
##
@@ -31,18 +31,6 @@ public class ClusterStateUtil {
private static final int TIMEOUT_POLL_MS = 1000;
- /**
- * Wait to see *all* cores live and active.
- *
- * @param zkStateReader to use for ClusterState
- * @param timeoutInMs how long to wait before giving up
- * @return false if timed out
- */
- public static boolean waitForAllActiveAndLiveReplicas(
Review Comment:
keep
--
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]
Re: [PR] Test framework code review and clean ups [solr]
dsmiley commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2589183175
##
solr/test-framework/src/java/org/apache/solr/util/SolrClientTestRule.java:
##
@@ -85,11 +84,6 @@ public NewCollectionBuilder withConfigSet(String configSet) {
return this;
}
-public NewCollectionBuilder withConfigFile(String configFile) {
Review Comment:
BTW, thanks for drawing attention to some of the removals in the PR comment!
It's a murky area that will be subjective; I don't think I can articulate
guidance/recommendation. I will review them and comment on any that I suggest
that you keep.
--
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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on PR #3920: URL: https://github.com/apache/solr/pull/3920#issuecomment-3611851477 I jsut updated the description to try and focus on the methods removed that might be used by external consumers of the framework... -- 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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2588750417
##
solr/test-framework/src/java/org/apache/solr/util/SolrClientTestRule.java:
##
@@ -85,11 +84,6 @@ public NewCollectionBuilder withConfigSet(String configSet) {
return this;
}
-public NewCollectionBuilder withConfigFile(String configFile) {
Review Comment:
So, I think this is the specific feedback that I'm looking for... should we
keep methods that aren't used by Solr in the test-framework? My default is to
remove code that isn't used by SOlr actively under the theory that fewer lines
of code reduces tech burden and makes it easier to understand these classes for
both me and any one else who is new to Solr.. More code is harder. Your
comment though gets to the crux, should I keep the methods in test framework
that aren't used like these builders on the theory that others MIGHT be calling
them. And maybe, if they are more complex than getter/setter, we should up the
test coverage that we have for them in the `test-framework/src/test/` tree...
--
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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2588737271
##
solr/test-framework/src/java/org/apache/solr/cloud/ChaosMonkey.java:
##
@@ -77,7 +77,7 @@ public class ChaosMonkey {
// NOTE: CONN_LOSS and EXP are currently being set to "false" intentionally
here. Remove the
// default value once we know tests pass reliably under those conditions
private static final String CONN_LOSS =
- System.getProperty("solr.tests.cloud.cm.connloss", "false");
+ System.getProperty("solr.tests.cloud.cm.connloss.enabled", "false");
Review Comment:
I can live with it, I suspect no one uses this unless they are actively
working the topic, I don't think it's in a CI setup for example where a name
change would matter.
--
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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2588738424
##
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##
@@ -152,7 +152,7 @@ public class MiniSolrCloudCluster {
+ "\n";
private final Object startupWait = new Object();
- private volatile ZkTestServer zkServer; // non-final due to injectChaos()
+ private final ZkTestServer zkServer;
Review Comment:
Reverted. I don't have confidence in my understanding of when to use
volatile ;-)
##
solr/test-framework/src/java/org/apache/solr/util/RevertDefaultThreadHandlerRule.java:
##
@@ -35,7 +35,7 @@ protected void before() throws Throwable {
if (!applied.getAndSet(true)) {
UncaughtExceptionHandler p =
Thread.getDefaultUncaughtExceptionHandler();
try {
-// Try to initialize a zookeeper class that reinitializes default
exception handler.
+// Try to initialize a zookeeper class that reinitialized default
exception handler.
Review Comment:
Done!
--
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]
Re: [PR] Test framework code review and clean ups [solr]
epugh commented on PR #3920: URL: https://github.com/apache/solr/pull/3920#issuecomment-3611784030 The changes were me, I grinded it out as a break from writing up a talk ;-) The summary was AI... I wanted to separate what I thought would be the more contentious question: _removing unused methods_ for human review from the more day to day typos and other ide suggested fixes. So I asked AI to look at the commits and highlight the removed methods, as maybe we should keep them even if we don't call them as others building on top of test-framework might??? -- 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]
Re: [PR] Test framework code review and clean ups [solr]
dsmiley commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2587162898
##
solr/test-framework/src/java/org/apache/solr/util/SolrClientTestRule.java:
##
@@ -85,11 +84,6 @@ public NewCollectionBuilder withConfigSet(String configSet) {
return this;
}
-public NewCollectionBuilder withConfigFile(String configFile) {
Review Comment:
please keep this, by the way. It's a test utility class with various
helpers and options, not all of which are used in Solr's tests today.
--
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]
Re: [PR] Test framework code review and clean ups [solr]
dsmiley commented on code in PR #3920:
URL: https://github.com/apache/solr/pull/3920#discussion_r2587148461
##
solr/test-framework/src/java/org/apache/solr/util/SolrClientTestRule.java:
##
@@ -85,11 +84,6 @@ public NewCollectionBuilder withConfigSet(String configSet) {
return this;
}
-public NewCollectionBuilder withConfigFile(String configFile) {
Review Comment:
why was this removed?
Did "AI" do this or did you choose to do this?
My suspicion is that AI did this... (I'll continue my review as a PR comment)
--
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]
