You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/07/01 18:21:39 UTC
[GitHub] [solr] epugh opened a new pull request, #924: SOLR-16276: redundant variable in tests
epugh opened a new pull request, #924:
URL: https://github.com/apache/solr/pull/924
https://issues.apache.org/jira/browse/SOLR-16276
# Description
IntelliJ flags redundant variables... Sometimes this is a redundency that helps explain the test, so you would want that. Sometimes it's a magic number (looking at you timeout variables!) that are always the same, and often it would flag a code path that was never actually traversed (looking at you null's passed into parameters or boolean values).
# Solution
Review and made judgement calls.
This PR is ripe for bikeshedding, so please holler if you don't like one of the changes and I'll roll it back ;-).
# Tests
Ran tests
# Checklist
Please review the following and check all that apply:
- [ x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ x] I have created a Jira issue and added the issue ID to my pull request title.
- [ x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ x] I have developed this patch against the `main` branch.
- [ x] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #924:
URL: https://github.com/apache/solr/pull/924#issuecomment-1175010725
Thanks @cpoerschke for the reivew, I think you are most of the way but not all the way through right??? I *think* I've respnded toall of your feedback items.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913746653
##########
solr/core/src/test/org/apache/solr/update/processor/UUIDUpdateProcessorFallbackTest.java:
##########
@@ -155,17 +155,17 @@ SolrInputDocument doc(SolrInputField... fields) {
}
/** Convenience method for building up SolrInputFields */
- SolrInputField field(String name, float boost, Object... values) {
+ SolrInputField field(String name, Object... values) {
SolrInputField f = new SolrInputField(name);
for (Object v : values) {
f.addValue(v);
}
return f;
}
- /** Convenience method for building up SolrInputFields with default boost */
+ /** Convenience method for building up SolrInputFields */
SolrInputField f(String name, Object... values) {
- return field(name, 1.0F, values);
+ return field(name, values);
}
Review Comment:
So, I removed the `field` method since `f` is used everywhere....
I suspect that this pattern is spread across lots of tests, and it seems like moving some methods into some unit testing helpers might be good...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913195953
##########
solr/core/src/test/org/apache/solr/update/processor/UUIDUpdateProcessorFallbackTest.java:
##########
@@ -165,7 +165,7 @@ SolrInputField field(String name, float boost, Object... values) {
/** Convenience method for building up SolrInputFields with default boost */
Review Comment:
Can you elaborate? as in make `f` be `field`?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913697233
##########
solr/core/src/test/org/apache/solr/cloud/PeerSyncReplicationTest.java:
##########
@@ -289,15 +288,13 @@ private CloudJettyRunner forceNodeFailureAndDoPeerSync(boolean disableFingerprin
indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
commit();
- bringUpDeadNodeAndEnsureNoReplication(replicaToShutDown, disableFingerprint);
+ bringUpDeadNodeAndEnsureNoReplication(replicaToShutDown);
return replicaToShutDown;
}
- private void bringUpDeadNodeAndEnsureNoReplication(
- CloudJettyRunner nodeToBringUp, boolean disableFingerprint) throws Exception {
- // disable fingerprint check if needed
- System.setProperty("solr.disableFingerprint", String.valueOf(disableFingerprint));
Review Comment:
So, I think this NEVER got a value of true... The disableFingerprint was never set to TRUE in the tests... In fact, I found another place where we clear the property that we never set... You are probably right this shoiuld be it's own issue... on the other hand I'm right in it now... sigh..
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913194604
##########
solr/core/src/test/org/apache/solr/cloud/ReindexCollectionTest.java:
##########
@@ -167,24 +167,23 @@ public void testSameTargetReindexing() throws Exception {
private void doTestSameTargetReindexing(boolean sourceRemove, boolean followAliases)
throws Exception {
- final String sourceCollection = "sameTargetReindexing_" + sourceRemove + "_" + followAliases;
- final String targetCollection = sourceCollection;
Review Comment:
agreed... changing this back... Thanks!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913655672
##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -87,6 +87,7 @@
public class TestReplicationHandler extends SolrTestCaseJ4 {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ public static final long TIMEOUT = 30000;
Review Comment:
```suggestion
private static final long TIMEOUT = 30000;
```
##########
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java:
##########
@@ -1428,8 +1423,8 @@ private long uploadBadConfigSet(
configSetName,
suffix,
username,
- overwrite,
- cleanup,
+ true,
+ true,
Review Comment:
```suggestion
true /* overwrite */,
true /* cleanup */,
```
##########
solr/core/src/test/org/apache/solr/search/TestSearchPerf.java:
##########
@@ -249,7 +243,7 @@ public void XtestFilteringPerformance() throws Exception {
createIndex2(indexSize, "foomany_s", "t10_100_ws");
// doListGen(100, q, filters, false, true);
Review Comment:
```suggestion
```
##########
solr/core/src/test/org/apache/solr/update/processor/UUIDUpdateProcessorFallbackTest.java:
##########
@@ -155,17 +155,17 @@ SolrInputDocument doc(SolrInputField... fields) {
}
/** Convenience method for building up SolrInputFields */
- SolrInputField field(String name, float boost, Object... values) {
+ SolrInputField field(String name, Object... values) {
SolrInputField f = new SolrInputField(name);
for (Object v : values) {
f.addValue(v);
}
return f;
}
- /** Convenience method for building up SolrInputFields with default boost */
+ /** Convenience method for building up SolrInputFields */
SolrInputField f(String name, Object... values) {
- return field(name, 1.0F, values);
+ return field(name, values);
}
Review Comment:
So now the `f` method has the same signature as the `field` method, and since `f` only calls `field` the `f` method itself can be removed i.e. callers can directly call `field` instead.
##########
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java:
##########
@@ -1333,7 +1333,8 @@ public void testUploadWithLibDirective() throws Exception {
}
private static String getSecurityJson() throws KeeperException, InterruptedException {
- String securityJson =
+ String securityJson;
+ securityJson =
Review Comment:
```suggestion
return
```
##########
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java:
##########
@@ -1522,8 +1517,8 @@ private long uploadSingleConfigSetFile(
username,
usePut);
assertNotNull(map);
- long statusCode =
- (long) getObjectByPath(map, false, Arrays.asList("responseHeader", "status"));
+ long statusCode;
+ statusCode = (long) getObjectByPath(map, Arrays.asList("responseHeader", "status"));
return statusCode;
Review Comment:
```suggestion
return (long) getObjectByPath(map, Arrays.asList("responseHeader", "status"));
```
##########
solr/core/src/test/org/apache/solr/search/TestSearchPerf.java:
##########
@@ -153,31 +155,23 @@ int doSetGen(int iter, Query q) throws Exception {
return ret;
}
- int doListGen(int iter, Query q, List<Query> filt, boolean cacheQuery, boolean cacheFilt)
- throws Exception {
+ int doListGen(Query q, List<Query> filt) throws Exception {
Review Comment:
Since this method is not private, need we worry about maintaining the signature? Or if changing the signature anyhow, let's make it private at the same time?
```suggestion
private int doListGen(Query q, List<Query> filt) throws Exception {
```
##########
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java:
##########
@@ -1688,9 +1682,6 @@ private static Object getObjectByPath(
if (obj == null) return null;
} else {
Object val = obj.get(s);
- if (onlyPrimitive && val instanceof Map) {
- return null;
- }
Review Comment:
nice simplification!
##########
solr/core/src/test/org/apache/solr/search/TestCollapseQParserPlugin.java:
##########
@@ -431,8 +431,8 @@ private void testDoubleCollapse(String group, String hint) {
params = new ModifiableSolrParams();
params.add("q", "id:(1 2 5)");
- params.add("fq", "{!collapse cost=200 max=test_i field=term_s " + hint + "}");
- params.add("fq", "{!collapse cost=400 max=test_i field=" + group + "" + hint + "}");
+ params.add("fq", "{!collapse cost=200 max=test_i field=term_s " + "" + "}");
+ params.add("fq", "{!collapse cost=400 max=test_i field=" + group + "" + "" + "}");
Review Comment:
```suggestion
params.add("fq", "{!collapse cost=200 max=test_i field=term_s }");
params.add("fq", "{!collapse cost=400 max=test_i field=" + group + "}");
```
##########
solr/core/src/test/org/apache/solr/search/TestCancellableCollector.java:
##########
@@ -117,20 +117,11 @@ private void executeSearchTest(
assertEquals(topDocs.totalHits.value, topScoreDocCollector.getTotalHits());
}
- private void cancelQuery(CancellableCollector cancellableCollector, final int sleepTime) {
+ private void cancelQuery(CancellableCollector cancellableCollector) {
executor.submit(
() -> {
// Wait for some time to let the query start
Review Comment:
```suggestion
```
##########
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java:
##########
@@ -1464,8 +1459,8 @@ private long uploadGivenConfigSet(
username,
usePut);
assertNotNull(map);
- long statusCode =
- (long) getObjectByPath(map, false, Arrays.asList("responseHeader", "status"));
+ long statusCode;
+ statusCode = (long) getObjectByPath(map, Arrays.asList("responseHeader", "status"));
return statusCode;
Review Comment:
```suggestion
return (long) getObjectByPath(map, Arrays.asList("responseHeader", "status"));
```
##########
solr/core/src/test/org/apache/solr/search/TestSearchPerf.java:
##########
@@ -153,31 +155,23 @@ int doSetGen(int iter, Query q) throws Exception {
return ret;
}
- int doListGen(int iter, Query q, List<Query> filt, boolean cacheQuery, boolean cacheFilt)
- throws Exception {
+ int doListGen(Query q, List<Query> filt) throws Exception {
SolrQueryRequest req = lrf.makeRequest();
SolrIndexSearcher searcher = req.getSearcher();
final RTimer timer = new RTimer();
int ret = 0;
- for (int i = 0; i < iter; i++) {
+ for (int i = 0; i < ITERATIONS; i++) {
DocList l =
- searcher.getDocList(
- q,
- filt,
- (Sort) null,
- 0,
- 10,
- (cacheQuery ? 0 : SolrIndexSearcher.NO_CHECK_QCACHE)
- | (cacheFilt ? 0 : SolrIndexSearcher.NO_CHECK_FILTERCACHE));
+ searcher.getDocList(q, filt, (Sort) null, 0, 10, SolrIndexSearcher.NO_CHECK_QCACHE | 0);
Review Comment:
```suggestion
searcher.getDocList(q, filt, (Sort) null, 0, 10, SolrIndexSearcher.NO_CHECK_QCACHE);
```
##########
solr/core/src/test/org/apache/solr/request/TestStreamBody.java:
##########
@@ -127,17 +127,15 @@ public String getPath() { // don't let superclass substitute qt for the path
SolrException se =
expectThrows(SolrException.class, () -> queryRequest.process(getSolrClient()));
assertTrue(se.getMessage(), se.getMessage().contains("Stream Body is disabled"));
- enableStreamBody(true);
+ enableStreamBody();
queryRequest.process(getSolrClient());
}
// Enables/disables stream.body through Config API
Review Comment:
```suggestion
// Enables stream.body through Config API
```
##########
solr/core/src/test/org/apache/solr/handler/admin/CoreAdminCreateDiscoverTest.java:
##########
@@ -60,7 +60,7 @@ public static void afterClass() throws Exception {
solrHomeDirectory = null;
}
- private static void setupCore(String coreName, boolean blivet) throws IOException {
Review Comment:
Had to look that word up: https://en.wiktionary.org/wiki/blivet
##########
solr/core/src/test/org/apache/solr/handler/admin/TestApiFramework.java:
##########
@@ -361,7 +355,6 @@ private SolrQueryResponse invoke(
PluginBag<SolrRequestHandler> reqHandlers,
String path,
String fullPath,
- SolrRequest.METHOD method,
Review Comment:
Could we maybe keep the `method` parameter here and instead replace the `"GET"` below with `method.toString()` or something?
##########
solr/core/src/test/org/apache/solr/schema/TestManagedSchemaThreadSafety.java:
##########
@@ -52,10 +52,11 @@
public class TestManagedSchemaThreadSafety extends SolrTestCaseJ4 {
private static final class SuspendingZkClient extends SolrZkClient {
+ public static final int ZK_CLIENT_TIMEOUT = 30000;
Review Comment:
```suggestion
private static final int ZK_CLIENT_TIMEOUT = 30000;
```
##########
solr/core/src/test/org/apache/solr/search/TestSearchPerf.java:
##########
@@ -33,6 +33,8 @@
/** */
public class TestSearchPerf extends SolrTestCaseJ4 {
+ public static final int ITERATIONS = 500;
Review Comment:
```suggestion
private static final int ITERATIONS = 500;
```
##########
solr/core/src/test/org/apache/solr/search/TestCollapseQParserPlugin.java:
##########
@@ -421,8 +421,8 @@ private void testDoubleCollapse(String group, String hint) {
ModifiableSolrParams params = new ModifiableSolrParams();
params.add("q", "id:(1 2 5)");
- params.add("fq", "{!collapse cost=200 field=term_s " + hint + "}");
- params.add("fq", "{!collapse cost=400 field=" + group + "" + hint + "}");
+ params.add("fq", "{!collapse cost=200 field=term_s " + "" + "}");
+ params.add("fq", "{!collapse cost=400 field=" + group + "" + "" + "}");
Review Comment:
```suggestion
params.add("fq", "{!collapse cost=200 field=term_s }");
params.add("fq", "{!collapse cost=400 field=" + group + "}");
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913939802
##########
solr/core/src/test/org/apache/solr/schema/TestCloudSchemaless.java:
##########
@@ -63,7 +63,8 @@ protected String getCloudSolrConfig() {
@Override
public SortedMap<ServletHolder, String> getExtraServlets() {
- final SortedMap<ServletHolder, String> extraServlets = new TreeMap<>();
Review Comment:
Hmm, it perhaps didn't matter because it used the method from the base class? There's a little bit of `getExtraServlets` sprinkled across the code base -- https://github.com/apache/solr/search?q=getExtraServlets -- let's out-scope here and clean that up separately.
https://github.com/apache/solr/pull/924/commits/392f09ee184d61e92c8405d25ac0c25052b734d1 added to defer.
##########
solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java:
##########
@@ -1253,9 +1253,8 @@ public void testDateRangeFieldFacets() {
private void helpTestDateFacets(final String fieldName, final FacetRangeMethod rangeFacetMethod) {
Review Comment:
https://github.com/apache/solr/pull/924/commits/edd62f508b69bbd8147e693dfc208f2f35ccf4e5 pushed to the branch.
##########
solr/core/src/test/org/apache/solr/cloud/PeerSyncReplicationTest.java:
##########
@@ -289,15 +288,13 @@ private CloudJettyRunner forceNodeFailureAndDoPeerSync(boolean disableFingerprin
indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
commit();
- bringUpDeadNodeAndEnsureNoReplication(replicaToShutDown, disableFingerprint);
+ bringUpDeadNodeAndEnsureNoReplication(replicaToShutDown);
return replicaToShutDown;
}
- private void bringUpDeadNodeAndEnsureNoReplication(
- CloudJettyRunner nodeToBringUp, boolean disableFingerprint) throws Exception {
- // disable fingerprint check if needed
- System.setProperty("solr.disableFingerprint", String.valueOf(disableFingerprint));
Review Comment:
https://github.com/apache/solr/pull/924/commits/c20b02e64f1a6b3fde9b018845cad22c683534e5 added to defer please.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913914168
##########
solr/core/src/test/org/apache/solr/update/PeerSyncWithIndexFingerprintCachingTest.java:
##########
@@ -110,6 +110,6 @@ void assertSync(SolrClient client, int numVersions, boolean expectedResult, Stri
"sync",
StrUtils.join(Arrays.asList(syncWith), ',')));
NamedList<?> rsp = client.request(qr);
- assertEquals(expectedResult, rsp.get("sync"));
+ assertEquals(true, rsp.get("sync"));
Review Comment:
```suggestion
assertTrue(rsp.get("sync"));
```
##########
solr/core/src/test/org/apache/solr/update/TestInPlaceUpdateWithRouteField.java:
##########
@@ -160,9 +160,9 @@ private void checkWrongCommandFailure(SolrInputDocument sdoc)
}
}
- private Collection<SolrInputDocument> createDocs(int number) {
+ private Collection<SolrInputDocument> createDocs() {
List<SolrInputDocument> result = new ArrayList<>();
- for (int i = 0; i < number; i++) {
+ for (int i = 0; i < TestInPlaceUpdateWithRouteField.NUMBER_OF_DOCS; i++) {
Review Comment:
```suggestion
for (int i = 0; i < NUMBER_OF_DOCS; i++) {
```
##########
solr/core/src/test/org/apache/solr/update/PeerSyncWithLeaderAndIndexFingerprintCachingTest.java:
##########
@@ -41,6 +41,6 @@ void assertSync(SolrClient client, int numVersions, boolean expectedResult, Stri
StrUtils.join(Arrays.asList(syncWith), ',')));
@SuppressWarnings({"rawtypes"})
NamedList rsp = client.request(qr);
- assertEquals(expectedResult, (Boolean) rsp.get("syncWithLeader"));
+ assertEquals(true, (Boolean) rsp.get("syncWithLeader"));
Review Comment:
```suggestion
assertTrue(rsp.get("syncWithLeader"));
```
##########
solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java:
##########
@@ -1253,9 +1253,8 @@ public void testDateRangeFieldFacets() {
private void helpTestDateFacets(final String fieldName, final FacetRangeMethod rangeFacetMethod) {
Review Comment:
I'm gonna be awkward here and propose renaming of the parameter as an alternative i.e. smaller diff and keeps style of the test intact with the `p/b/f/c` etc. variable names style.
```suggestion
private void helpTestDateFacets(final String f, final FacetRangeMethod rangeFacetMethod) {
```
(I'll push change directly to the PR branch if that's alright.)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #924:
URL: https://github.com/apache/solr/pull/924#issuecomment-1175236948
running all the tests again, and tjhen will merge. Thanks @cpoerschke for the efforts!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913912286
##########
solr/core/src/test/org/apache/solr/update/MaxSizeAutoCommitTest.java:
##########
@@ -204,46 +201,44 @@ public void testDeletes() throws Exception {
updateHandler,
() -> {
updateRequestHandler.handleRequest(
- constructBatchDeleteDocRequest(0, numDocsToAdd), updateResp);
+ constructBatchDeleteDocRequest(numDocsToAdd), updateResp);
});
}
/**
* Construct a batch add document request with a series of very simple Solr docs with increasing
* IDs.
*
- * @param startId the document ID to begin with
* @param batchSize the number of documents to include in the batch
* @return a SolrQueryRequestBase
*/
- private SolrQueryRequestBase constructBatchAddDocRequest(int startId, int batchSize) {
- return constructBatchRequestHelper(startId, batchSize, MaxSizeAutoCommitTest::addDoc);
+ private SolrQueryRequestBase constructBatchAddDocRequest(int batchSize) {
+ return constructBatchRequestHelper(batchSize, MaxSizeAutoCommitTest::addDoc);
}
/**
* Construct a batch delete document request, with IDs incrementing from startId
*
- * @param startId the document ID to begin with
* @param batchSize the number of documents to include in the batch
* @return a SolrQueryRequestBase
*/
- private SolrQueryRequestBase constructBatchDeleteDocRequest(int startId, int batchSize) {
- return constructBatchRequestHelper(startId, batchSize, MaxSizeAutoCommitTest::delDoc);
+ private SolrQueryRequestBase constructBatchDeleteDocRequest(int batchSize) {
+ return constructBatchRequestHelper(batchSize, MaxSizeAutoCommitTest::delDoc);
}
/**
* Helper for constructing a batch update request
*
- * @param startId the document ID to begin with
* @param batchSize the number of documents to include in the batch
* @param requestFn a function that takes an (int) ID and returns an XML string of the request to
* add to the batch request
* @return a SolrQueryRequestBase
*/
private SolrQueryRequestBase constructBatchRequestHelper(
- int startId, int batchSize, Function<Integer, String> requestFn) {
+ int batchSize, Function<Integer, String> requestFn) {
SolrQueryRequestBase updateReq =
new SolrQueryRequestBase(core, new MapSolrParams(new HashMap<>())) {};
+ int startId = 0;
List<String> docs = new ArrayList<>();
for (int i = startId; i < startId + batchSize; i++) {
Review Comment:
```suggestion
List<String> docs = new ArrayList<>();
for (int i = 0; i < batchSize; i++) {
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913913233
##########
solr/core/src/test/org/apache/solr/update/PeerSyncWithBufferUpdatesTest.java:
##########
@@ -227,6 +227,6 @@ void assertSync(SolrClient client, int numVersions, boolean expectedResult, Stri
"syncWithLeader",
syncWith));
NamedList<?> rsp = client.request(qr);
- assertEquals(expectedResult, rsp.get("syncWithLeader"));
+ assertEquals(true, rsp.get("syncWithLeader"));
Review Comment:
```suggestion
assertTrue(rsp.get("syncWithLeader"));
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913714990
##########
solr/core/src/test/org/apache/solr/handler/component/StatsComponentTest.java:
##########
@@ -1248,17 +1248,16 @@ public void testMiscQueryStats() throws Exception {
// force constant score for matches, so we aren't dependent on similarity
final float constScore = 4.2F;
- final double expectedScore = (double) constScore;
Review Comment:
makes sense...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913712611
##########
solr/core/src/test/org/apache/solr/schema/TestCloudSchemaless.java:
##########
@@ -63,7 +63,8 @@ protected String getCloudSolrConfig() {
@Override
public SortedMap<ServletHolder, String> getExtraServlets() {
- final SortedMap<ServletHolder, String> extraServlets = new TreeMap<>();
Review Comment:
I took the method out and it didn't matter, can we consider it a "redundent method" and include it in the PR ;-)??
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh merged pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
epugh merged PR #924:
URL: https://github.com/apache/solr/pull/924
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r912746051
##########
solr/core/src/test/org/apache/solr/cloud/OverseerCollectionConfigSetProcessorTest.java:
##########
@@ -103,6 +103,7 @@ public class OverseerCollectionConfigSetProcessorTest extends SolrTestCaseJ4 {
private static final String ADMIN_PATH = "/admin/cores";
private static final String COLLECTION_NAME = "mycollection";
private static final String CONFIG_NAME = "myconfig";
+ public static final int MAXWAIT = 10000;
Review Comment:
```suggestion
private static final int MAX_WAIT_MS = 10000;
```
##########
solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java:
##########
@@ -77,6 +77,7 @@
@LuceneTestCase.Slow
public class CollectionsAPISolrJTest extends SolrCloudTestCase {
+ public static final int TIMEOUT = 3000;
Review Comment:
```suggestion
private static final int TIMEOUT = 3000;
```
##########
solr/core/src/test/org/apache/solr/cloud/PeerSyncReplicationTest.java:
##########
@@ -289,15 +288,13 @@ private CloudJettyRunner forceNodeFailureAndDoPeerSync(boolean disableFingerprin
indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
commit();
- bringUpDeadNodeAndEnsureNoReplication(replicaToShutDown, disableFingerprint);
+ bringUpDeadNodeAndEnsureNoReplication(replicaToShutDown);
return replicaToShutDown;
}
- private void bringUpDeadNodeAndEnsureNoReplication(
- CloudJettyRunner nodeToBringUp, boolean disableFingerprint) throws Exception {
- // disable fingerprint check if needed
- System.setProperty("solr.disableFingerprint", String.valueOf(disableFingerprint));
Review Comment:
This looks like a logic change? Suggest to defer i.e. to out-scope from this code cleanup round.
##########
solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java:
##########
@@ -210,7 +210,8 @@ public void testIntersectFilter() throws Exception {
public void checkResultFormat() {
// Check input and output format is the same
String IN = "89.9,-130"; // lat,lon
Review Comment:
How about combining the two variables?
```suggestion
final String IN_OUT = "89.9,-130"; // lat,lon
```
##########
solr/core/src/test/org/apache/solr/handler/component/StatsComponentTest.java:
##########
@@ -1248,17 +1248,16 @@ public void testMiscQueryStats() throws Exception {
// force constant score for matches, so we aren't dependent on similarity
final float constScore = 4.2F;
- final double expectedScore = (double) constScore;
Review Comment:
subjective: clearer before IMO i.e. fewer casts and clear that it's the expected score
##########
solr/core/src/test/org/apache/solr/schema/TestCloudSchemaless.java:
##########
@@ -63,7 +63,8 @@ protected String getCloudSolrConfig() {
@Override
public SortedMap<ServletHolder, String> getExtraServlets() {
- final SortedMap<ServletHolder, String> extraServlets = new TreeMap<>();
Review Comment:
I think I'd prefer how it was before, but that's subjective. Separately (and out of the scope of this pull request) wondering if the `getExtraServlets` stuff within the code base is still needed or if it can be removed perhaps.
##########
solr/core/src/test/org/apache/solr/cloud/OverseerCollectionConfigSetProcessorTest.java:
##########
@@ -894,10 +895,10 @@ protected void verifySubmitCaptures(
}
}
- protected void waitForEmptyQueue(long maxWait) throws Exception {
- final TimeOut timeout = new TimeOut(maxWait, TimeUnit.MILLISECONDS, TimeSource.NANO_TIME);
+ protected void waitForEmptyQueue() throws Exception {
+ final TimeOut timeout = new TimeOut(MAXWAIT, TimeUnit.MILLISECONDS, TimeSource.NANO_TIME);
while (queue.peek() != null) {
- if (timeout.hasTimedOut()) fail("Queue not empty within " + maxWait + " ms");
+ if (timeout.hasTimedOut()) fail("Queue not empty within " + (long) 10000 + " ms");
Review Comment:
```suggestion
if (timeout.hasTimedOut()) fail("Queue not empty within " + MAX_WAIT_MS + " ms");
```
##########
solr/core/src/test/org/apache/solr/cloud/CollectionPropsTest.java:
##########
@@ -45,6 +45,7 @@
@LuceneTestCase.Slow
@SolrTestCaseJ4.SuppressSSL
public class CollectionPropsTest extends SolrCloudTestCase {
+ public static final int TIMEOUT = 5000;
Review Comment:
might `private` be sufficient?
```suggestion
private static final int TIMEOUT = 5000;
```
##########
solr/core/src/test/org/apache/solr/update/processor/UUIDUpdateProcessorFallbackTest.java:
##########
@@ -165,7 +165,7 @@ SolrInputField field(String name, float boost, Object... values) {
/** Convenience method for building up SolrInputFields with default boost */
Review Comment:
```suggestion
/** Convenience method for building up SolrInputFields */
```
Does this mean then that `f` can be removed in favour of `field` actually?
##########
solr/core/src/test/org/apache/solr/cloud/ReindexCollectionTest.java:
##########
@@ -167,24 +167,23 @@ public void testSameTargetReindexing() throws Exception {
private void doTestSameTargetReindexing(boolean sourceRemove, boolean followAliases)
throws Exception {
- final String sourceCollection = "sameTargetReindexing_" + sourceRemove + "_" + followAliases;
- final String targetCollection = sourceCollection;
Review Comment:
source vs. target collection naming seems helpful here w.r.t. test logic readability
##########
solr/core/src/test/org/apache/solr/handler/component/ShardsAllowListTest.java:
##########
@@ -85,7 +85,8 @@ public static void setupClusters() throws Exception {
@Override
public MiniSolrCloudCluster apply(String clusterId) {
try {
- final MiniSolrCloudCluster cluster =
+ final MiniSolrCloudCluster cluster;
+ cluster =
Review Comment:
```suggestion
return
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #924: SOLR-16276: redundant variable in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r913729702
##########
solr/core/src/test/org/apache/solr/update/processor/UUIDUpdateProcessorFallbackTest.java:
##########
@@ -155,17 +155,17 @@ SolrInputDocument doc(SolrInputField... fields) {
}
/** Convenience method for building up SolrInputFields */
- SolrInputField field(String name, float boost, Object... values) {
+ SolrInputField field(String name, Object... values) {
SolrInputField f = new SolrInputField(name);
for (Object v : values) {
f.addValue(v);
}
return f;
}
- /** Convenience method for building up SolrInputFields with default boost */
+ /** Convenience method for building up SolrInputFields */
SolrInputField f(String name, Object... values) {
- return field(name, 1.0F, values);
+ return field(name, values);
}
Review Comment:
looks like it's a convenience method, `doc(f("name", "Existing", "Values"), f("id", "75765"))`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org