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/26 17:07:04 UTC
[GitHub] [solr] epugh opened a new pull request, #953: SOLR-16311: Simplify assert statement
epugh opened a new pull request, #953:
URL: https://github.com/apache/solr/pull/953
https://issues.apache.org/jira/browse/SOLR-16311
# Description
This may be very stylistic in a nature, some people may like the `assertTrue` used everywhere, instead of using the various condition specific asserts like `assertArrayEquals` etc...
However, these were flagged... And some of our logic is pretty complex!
# Solution
Use the simplified version mentioned by intellij
# Tests
ran them
# Checklist
Please review the following and check all that apply:
- [ ] 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.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] 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)
- [ ] I have developed this patch against the `main` branch.
- [ ] 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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998701533
##########
solr/modules/ltr/src/test/org/apache/solr/ltr/norm/TestMinMaxNormalizer.java:
##########
@@ -16,17 +16,14 @@
*/
package org.apache.solr.ltr.norm;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
import java.nio.file.Paths;
import java.util.HashMap;
import java.util.Map;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.core.SolrResourceLoader;
import org.junit.Test;
-public class TestMinMaxNormalizer {
+public class TestMinMaxNormalizer extends SolrTestCaseJ4 {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/modules/ltr/src/test/org/apache/solr/ltr/norm/TestStandardNormalizer.java:
##########
@@ -16,17 +16,14 @@
*/
package org.apache.solr.ltr.norm;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
import java.nio.file.Paths;
import java.util.HashMap;
import java.util.Map;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.core.SolrResourceLoader;
import org.junit.Test;
-public class TestStandardNormalizer {
+public class TestStandardNormalizer extends SolrTestCaseJ4 {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998869165
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/eval/ArrayEvaluatorTest.java:
##########
@@ -54,12 +53,12 @@ public void arrayLongSortAscTest() throws IOException {
result = evaluator.evaluate(new Tuple(values));
- Assert.assertTrue(result instanceof List<?>);
+ assertTrue(result instanceof List<?>);
- Assert.assertEquals(3, ((List<?>) result).size());
Review Comment:
Fixed in 3e46998f859ac463dd68c75f35fe177fba1323a2
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r999871914
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -425,22 +424,22 @@ public void testBasicSelect() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 3);
+ assertEquals(3, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("myId") == 3);
- assert (tuple.getLong("myInt") == 20);
- assert (tuple.get("myString").equals("a"));
+ assertEquals(3, (long) tuple.getLong("myId"));
Review Comment:
@mkhludnev shared `assertEquals(3, tuple.getLong("myId").longValue());` which is even better than what I could come up with.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998772151
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -2308,43 +2308,43 @@ public void testParallelTimeSeriesGrouping() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 6);
+ assertEquals(6, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("year_i") == 2015);
- assert (tuple.getLong("month_i") == 11);
- assert (tuple.getLong("day_i") == 8);
- assert (tuple.getDouble("EXPR$3") == 42); // sum(item_i)
+ assertEquals(2015, (long) tuple.getLong("year_i"));
Review Comment:
I think so - I'll take a look
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java:
##########
@@ -1075,15 +1075,15 @@ public void testGatherNodesFriendsStream() throws Exception {
tuples = getTuples(stream);
tuples.sort(new FieldComparator("node", ComparatorOrder.ASCENDING));
- assertTrue(tuples.size() == 4);
- assertTrue(tuples.get(0).getString("node").equals("bill"));
- assertTrue(tuples.get(0).getLong("level").equals(0L));
- assertTrue(tuples.get(1).getString("node").equals("jim"));
- assertTrue(tuples.get(1).getLong("level").equals(1L));
- assertTrue(tuples.get(2).getString("node").equals("max"));
- assertTrue(tuples.get(2).getLong("level").equals(1L));
- assertTrue(tuples.get(3).getString("node").equals("sam"));
- assertTrue(tuples.get(3).getLong("level").equals(1L));
+ assertEquals(4, tuples.size());
+ assertEquals("bill", tuples.get(0).getString("node"));
+ assertEquals(0L, (long) tuples.get(0).getLong("level"));
Review Comment:
I think so - I'll take a look
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/MathExpressionTest.java:
##########
@@ -1719,10 +1719,10 @@ public void testMatrix() throws Exception {
assertEquals(features.get(0).get(0), "col3");
assertEquals(features.get(1).get(0), "col1");
- assertTrue(tuples.get(0).getLong("f") == 2);
- assertTrue(tuples.get(0).getLong("g") == 3);
- assertTrue(tuples.get(0).getLong("h") == 1);
- assertTrue(tuples.get(0).getLong("i") == 2);
+ assertEquals(2, (long) tuples.get(0).getLong("f"));
Review Comment:
I think so - I'll take a look
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998699376
##########
solr/core/src/test/org/apache/solr/update/processor/SkipExistingDocumentsProcessorFactoryTest.java:
##########
@@ -38,7 +36,7 @@
import org.junit.Test;
import org.mockito.Mockito;
-public class SkipExistingDocumentsProcessorFactoryTest {
+public class SkipExistingDocumentsProcessorFactoryTest extends SolrTestCaseJ4 {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/core/src/test/org/apache/solr/util/SolrCliUptimeTest.java:
##########
@@ -16,11 +16,10 @@
*/
package org.apache.solr.util;
-import static org.junit.Assert.assertEquals;
-
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class SolrCliUptimeTest {
+public class SolrCliUptimeTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/core/src/test/org/apache/solr/util/hll/NumberUtilTest.java:
##########
@@ -16,13 +16,11 @@
*/
package org.apache.solr.util.hll;
-import static org.junit.Assert.assertTrue;
-
-import java.util.Arrays;
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
/** Tests {@link NumberUtil} */
-public class NumberUtilTest {
+public class NumberUtilTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r997711826
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -287,13 +286,13 @@ public void testBasicSelect() throws Exception {
tuple = tuples.get(7);
assertEquals(tuple.getLong("id").longValue(), 1);
assertEquals(tuple.getLong("field_i").longValue(), 7);
- assert (tuple.get("str_s").equals("a"));
+ assertEquals("a", tuple.get("str_s"));
assertEquals(tuple.getLong("field_i").longValue(), 7);
assertEquals(tuple.getDouble("field_f"), 7.5, 0.0);
assertEquals(tuple.getDouble("field_d"), 7.5, 0.0);
assertEquals(tuple.getLong("field_l").longValue(), 7);
- // Assert field order
+ // assertTruefield order
Review Comment:
Fix 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: 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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998703961
##########
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java:
##########
@@ -111,7 +108,7 @@ public static void assertVersions(SolrClient client1, SolrClient client2) throws
Long maxVersionClient2 = getVersion(client2);
if (maxVersionClient1 > 0 && maxVersionClient2 > 0) {
- assertEquals(maxVersionClient1, maxVersionClient2);
+ SolrTestCaseJ4.assertEquals(maxVersionClient1, maxVersionClient2);
Review Comment:
its a test helper. no real tests so seemed weird
--
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] risdenk commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1284423921
@mkhludnev so two items from your review that I haven't addressed. One I don't think I can fix. The other I'm open to changing to static imports if really desired.
Changes like `assertEquals(3, (long) tuple.getLong("myId"));` - I left a comment about how I don't see a way to fix this outside of doing `Long.valueOf(3)` which seems worse. Are you ok with going ahead with this PR without this being changed to not have a cast?
The second being changes like `SolrTestCaseJ4.assertNotNull` where I had removed the static import and qualified these. How strongly do you feel about these being `assertNotNull` vs leaving the `SolrTestCaseJ4.assertNotNull` qualification.
--
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] risdenk commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1284581400
While reviewing this change and the casts - found there were a smattering of raw `assert ...` in tests that really should be `assertTrue(...)` and then those could be simplified. So 18520354d31a393b975bad339a1d7ca740a60194 takes care of that.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998774152
##########
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java:
##########
@@ -111,7 +108,7 @@ public static void assertVersions(SolrClient client1, SolrClient client2) throws
Long maxVersionClient2 = getVersion(client2);
if (maxVersionClient1 > 0 && maxVersionClient2 > 0) {
- assertEquals(maxVersionClient1, maxVersionClient2);
+ SolrTestCaseJ4.assertEquals(maxVersionClient1, maxVersionClient2);
Review Comment:
This also goes for a few others (I think ~5 classes) that are test utils only and seemed weird to make extend `SolrTestCase`.
--
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] mkhludnev commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
mkhludnev commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r999858122
##########
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java:
##########
@@ -202,15 +199,16 @@ public static NamedList<Object> getDetails(SolrClient s) throws Exception {
@SuppressWarnings("unchecked")
NamedList<Object> details = (NamedList<Object>) res.get("details");
- assertNotNull("null details", details);
+ SolrTestCaseJ4.assertNotNull("null details", details);
return details;
}
public static void assertReplicationResponseSucceeded(NamedList<?> response) {
- assertNotNull("null response from server", response);
- assertNotNull("Expected replication response to have 'status' field", response.get("status"));
- assertEquals("OK", response.get("status"));
+ SolrTestCaseJ4.assertNotNull("null response from server", response);
Review Comment:
let's keep your option @risdenk
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998867130
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java:
##########
@@ -1075,15 +1075,15 @@ public void testGatherNodesFriendsStream() throws Exception {
tuples = getTuples(stream);
tuples.sort(new FieldComparator("node", ComparatorOrder.ASCENDING));
- assertTrue(tuples.size() == 4);
- assertTrue(tuples.get(0).getString("node").equals("bill"));
- assertTrue(tuples.get(0).getLong("level").equals(0L));
- assertTrue(tuples.get(1).getString("node").equals("jim"));
- assertTrue(tuples.get(1).getLong("level").equals(1L));
- assertTrue(tuples.get(2).getString("node").equals("max"));
- assertTrue(tuples.get(2).getLong("level").equals(1L));
- assertTrue(tuples.get(3).getString("node").equals("sam"));
- assertTrue(tuples.get(3).getLong("level").equals(1L));
+ assertEquals(4, tuples.size());
+ assertEquals("bill", tuples.get(0).getString("node"));
+ assertEquals(0L, (long) tuples.get(0).getLong("level"));
Review Comment:
Hmmm so `tuple.getLong` returns a `Long` because it also returns `null` sometimes. Changing to `long` can't be done for `tuple.getLong` since then `null` can't be returned. `3L` is still a `long` and can't be compared with `Long` apparently. I could change it to
```
assertEquals(Long.valueOf(3), tuple.getLong("myId"))
```
but that seems worse.
I don't have any other ideas right now :/
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/MathExpressionTest.java:
##########
@@ -1719,10 +1719,10 @@ public void testMatrix() throws Exception {
assertEquals(features.get(0).get(0), "col3");
assertEquals(features.get(1).get(0), "col1");
- assertTrue(tuples.get(0).getLong("f") == 2);
- assertTrue(tuples.get(0).getLong("g") == 3);
- assertTrue(tuples.get(0).getLong("h") == 1);
- assertTrue(tuples.get(0).getLong("i") == 2);
+ assertEquals(2, (long) tuples.get(0).getLong("f"));
Review Comment:
Hmmm so `tuple.getLong` returns a `Long` because it also returns `null` sometimes. Changing to `long` can't be done for `tuple.getLong` since then `null` can't be returned. `3L` is still a `long` and can't be compared with `Long` apparently. I could change it to
```
assertEquals(Long.valueOf(3), tuple.getLong("myId"))
```
but that seems worse.
I don't have any other ideas right now :/
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998704313
##########
solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:
##########
@@ -71,7 +66,8 @@ public static void beforeClass() {
}
@Before
- public void setUp() {
+ public void setUp() throws Exception {
+ super.setUp();
Review Comment:
lucene checks this. and protects against future use of anything from ancestor
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r999567154
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -425,22 +424,22 @@ public void testBasicSelect() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 3);
+ assertEquals(3, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("myId") == 3);
- assert (tuple.getLong("myInt") == 20);
- assert (tuple.get("myString").equals("a"));
+ assertEquals(3, (long) tuple.getLong("myId"));
Review Comment:
I looked at this again this morning - I don't know why auto boxing/unboxing isn't doing the right thing to compare `Long` with `long`. The explicit cast just makes the compiler happy. I think it is because `assertEquals` is checking two `Object`s in this case and doesn't seem to know to simplify to two `long` comparisons without the `(long)` cast. :(
--
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] mkhludnev commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
mkhludnev commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998298633
##########
solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:
##########
@@ -44,7 +39,7 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class BlobRepositoryMockingTest {
+public class BlobRepositoryMockingTest extends SolrTestCaseJ4 {
Review Comment:
Whu, so? it seems doubtful. I'd rather avoid this change
##########
solr/core/src/test/org/apache/solr/core/backup/BackupFilePathsTest.java:
##########
@@ -16,16 +16,13 @@
*/
package org.apache.solr.core.backup;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
import java.util.List;
import java.util.Optional;
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
/** Unit tests for {@link BackupFilePaths} */
-public class BackupFilePathsTest {
+public class BackupFilePathsTest extends SolrTestCase {
Review Comment:
Still not getting why
##########
solr/core/src/test/org/apache/solr/handler/TestSampleDocumentsLoader.java:
##########
@@ -40,7 +35,7 @@
import org.junit.Before;
import org.junit.Test;
-public class TestSampleDocumentsLoader {
+public class TestSampleDocumentsLoader extends SolrTestCase {
Review Comment:
disagree
##########
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java:
##########
@@ -111,7 +108,7 @@ public static void assertVersions(SolrClient client1, SolrClient client2) throws
Long maxVersionClient2 = getVersion(client2);
if (maxVersionClient1 > 0 && maxVersionClient2 > 0) {
- assertEquals(maxVersionClient1, maxVersionClient2);
+ SolrTestCaseJ4.assertEquals(maxVersionClient1, maxVersionClient2);
Review Comment:
Why not JUnit's one?
##########
solr/core/src/test/org/apache/solr/handler/V2ClusterAPIMappingTest.java:
##########
@@ -48,7 +47,7 @@
import org.mockito.ArgumentCaptor;
/** Unit tests for the v2 to v1 API mappings found in {@link ClusterAPI} */
-public class V2ClusterAPIMappingTest {
+public class V2ClusterAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
again, why?
##########
solr/core/src/test/org/apache/solr/handler/V2UpdateAPIMappingTest.java:
##########
@@ -39,7 +38,7 @@
import org.junit.Test;
/** Unit tests for the v2 to v1 mapping logic in {@link UpdateAPI} */
-public class V2UpdateAPIMappingTest {
+public class V2UpdateAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
🥱
##########
solr/core/src/test/org/apache/solr/handler/admin/api/V2NodeAPIMappingTest.java:
##########
@@ -55,7 +53,7 @@
import org.mockito.ArgumentCaptor;
/** Unit tests for the v2 to v1 mapping for /node/ APIs. */
-public class V2NodeAPIMappingTest {
+public class V2NodeAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
why?
##########
solr/core/src/test/org/apache/solr/parser/SolrQueryParserBaseTest.java:
##########
@@ -26,19 +24,17 @@
import java.util.List;
import org.apache.lucene.queryparser.charstream.CharStream;
import org.apache.lucene.search.Query;
+import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.common.SolrException;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.schema.IndexSchema;
import org.apache.solr.search.QParser;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
+import org.mockito.Mockito;
-@RunWith(MockitoJUnitRunner.class)
-public class SolrQueryParserBaseTest {
+public class SolrQueryParserBaseTest extends SolrTestCaseJ4 {
Review Comment:
Disagree with ancestor.
Why not @Mock, it seemed fine?
##########
solr/core/src/test/org/apache/solr/handler/component/SpellCheckComponentTest.java:
##########
@@ -640,7 +640,7 @@ public void testThresholdTokenFrequency() throws Exception {
values = rsp.getValues();
spellCheck = (NamedList<?>) values.get("spellcheck");
suggestions = (NamedList<?>) spellCheck.get("suggestions");
- assertTrue(suggestions.get("suggestion") == null);
- assertTrue((Boolean) spellCheck.get("correctlySpelled") == false);
+ assertNull(suggestions.get("suggestion"));
+ assertFalse((boolean) (Boolean) spellCheck.get("correctlySpelled"));
Review Comment:
(two) (casts)
##########
solr/core/src/test/org/apache/solr/update/SoftAutoCommitTest.java:
##########
@@ -637,6 +636,6 @@ public void clear() {
}
public void assertSaneOffers() {
- assertEquals("Failure of MockEventListener" + fail.toString(), 0, fail.length());
+ SolrTestCaseJ4.assertEquals("Failure of MockEventListener" + fail.toString(), 0, fail.length());
Review Comment:
Why classname is added?
##########
solr/core/src/test/org/apache/solr/util/hll/NumberUtilTest.java:
##########
@@ -70,65 +68,62 @@ public class NumberUtilTest {
@Test
public void testLog2() {
final double log2Result = NumberUtil.log2(2d);
- assertTrue(log2Result == 1);
+ assertEquals(1, log2Result, 0.0);
Review Comment:
if it's a double it might require epsilon. But I might miss a zero point
##########
solr/core/src/test/org/apache/solr/util/SolrCliUptimeTest.java:
##########
@@ -16,11 +16,10 @@
*/
package org.apache.solr.util;
-import static org.junit.Assert.assertEquals;
-
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class SolrCliUptimeTest {
+public class SolrCliUptimeTest extends SolrTestCase {
Review Comment:
Why?
##########
solr/core/src/test/org/apache/solr/handler/TestSampleDocumentsLoader.java:
##########
@@ -63,7 +58,7 @@ public void testJson() throws Exception {
@Test
public void testCsv() throws Exception {
ModifiableSolrParams params = new ModifiableSolrParams();
- params.set(CSV_MULTI_VALUE_DELIM_PARAM, "\\|");
+ params.set(DefaultSampleDocumentsLoader.CSV_MULTI_VALUE_DELIM_PARAM, "\\|");
Review Comment:
I'm not sure if it gets better
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -1915,17 +1914,17 @@ public void testTimeSeriesGrouping() throws Exception {
List<Tuple> tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 2);
+ assertEquals(2, tuples.size());
Tuple tuple;
tuple = tuples.get(0);
- assert (tuple.getLong("year_i") == 2015);
- assert (tuple.getDouble("EXPR$1") == 66); // sum(item_i)
+ assertEquals(2015, (long) tuple.getLong("year_i"));
+ assertEquals(66, tuple.getDouble("EXPR$1"), 0.0); // sum(item_i)
tuple = tuples.get(1);
- assert (tuple.getLong("year_i") == 2014);
- assert (tuple.getDouble("EXPR$1") == 7); // sum(item_i)
+ assertEquals(2014, (long) tuple.getLong("year_i"));
Review Comment:
can it be 2014L with no cast (long)?
##########
solr/modules/ltr/src/test/org/apache/solr/ltr/norm/TestMinMaxNormalizer.java:
##########
@@ -16,17 +16,14 @@
*/
package org.apache.solr.ltr.norm;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
import java.nio.file.Paths;
import java.util.HashMap;
import java.util.Map;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.core.SolrResourceLoader;
import org.junit.Test;
-public class TestMinMaxNormalizer {
+public class TestMinMaxNormalizer extends SolrTestCaseJ4 {
Review Comment:
disagree
##########
solr/modules/langid/src/test/org/apache/solr/update/processor/SolrInputDocumentReaderTest.java:
##########
@@ -16,21 +16,19 @@
*/
package org.apache.solr.update.processor;
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
import java.util.Arrays;
+import org.apache.solr.SolrTestCase;
import org.apache.solr.common.SolrInputDocument;
import org.junit.Before;
import org.junit.Test;
-public class SolrInputDocumentReaderTest {
+public class SolrInputDocumentReaderTest extends SolrTestCase {
Review Comment:
Why?
##########
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java:
##########
@@ -202,15 +199,16 @@ public static NamedList<Object> getDetails(SolrClient s) throws Exception {
@SuppressWarnings("unchecked")
NamedList<Object> details = (NamedList<Object>) res.get("details");
- assertNotNull("null details", details);
+ SolrTestCaseJ4.assertNotNull("null details", details);
return details;
}
public static void assertReplicationResponseSucceeded(NamedList<?> response) {
- assertNotNull("null response from server", response);
- assertNotNull("Expected replication response to have 'status' field", response.get("status"));
- assertEquals("OK", response.get("status"));
+ SolrTestCaseJ4.assertNotNull("null response from server", response);
Review Comment:
it's getting worse imho
##########
solr/core/src/test/org/apache/solr/handler/admin/SolrEnvironmentTest.java:
##########
@@ -17,13 +17,11 @@
package org.apache.solr.handler.admin;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-
+import org.apache.solr.SolrTestCase;
import org.apache.solr.common.SolrException;
import org.junit.Test;
-public class SolrEnvironmentTest {
+public class SolrEnvironmentTest extends SolrTestCase {
Review Comment:
please explain
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -425,22 +424,22 @@ public void testBasicSelect() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 3);
+ assertEquals(3, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("myId") == 3);
- assert (tuple.getLong("myInt") == 20);
- assert (tuple.get("myString").equals("a"));
+ assertEquals(3, (long) tuple.getLong("myId"));
Review Comment:
Can we avoid (long) cast with `3L` ?
##########
solr/core/src/test/org/apache/solr/handler/component/SpellCheckComponentTest.java:
##########
@@ -627,8 +627,8 @@ public void testThresholdTokenFrequency() throws Exception {
NamedList<?> values = rsp.getValues();
NamedList<?> spellCheck = (NamedList<?>) values.get("spellcheck");
NamedList<?> suggestions = (NamedList<?>) spellCheck.get("suggestions");
- assertTrue(suggestions.get("suggestion") == null);
- assertTrue((Boolean) spellCheck.get("correctlySpelled") == false);
+ assertNull(suggestions.get("suggestion"));
+ assertFalse((boolean) (Boolean) spellCheck.get("correctlySpelled"));
Review Comment:
these (two) (casts) seem weird
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientTest.java:
##########
@@ -17,22 +17,19 @@
package org.apache.solr.client.solrj.impl;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import org.apache.lucene.tests.util.LuceneTestCase;
+import org.apache.solr.SolrTestCase;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.request.QueryRequest;
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.junit.Test;
-public class LBSolrClientTest {
+public class LBSolrClientTest extends SolrTestCase {
Review Comment:
Why?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttpSolrClientTest.java:
##########
@@ -16,17 +16,15 @@
*/
package org.apache.solr.client.solrj.impl;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-
import java.io.IOException;
import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.solr.SolrTestCase;
import org.apache.solr.client.solrj.ResponseParser;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.junit.Test;
/** Test the LBHttpSolrClient. */
-public class LBHttpSolrClientTest {
+public class LBHttpSolrClientTest extends SolrTestCase {
Review Comment:
Why?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrPortAwareCookieSpecTest.java:
##########
@@ -21,11 +21,10 @@
import org.apache.http.cookie.MalformedCookieException;
import org.apache.http.impl.cookie.BasicClientCookie;
import org.apache.solr.SolrTestCaseJ4;
-import org.junit.Assert;
import org.junit.Test;
// Test cases imported from TestNetscapeCookieAttribHandlers of HttpClient project
-public class SolrPortAwareCookieSpecTest {
+public class SolrPortAwareCookieSpecTest extends SolrTestCaseJ4 {
Review Comment:
Why not Assert?
##########
solr/core/src/test/org/apache/solr/metrics/DelegateRegistryTimerTest.java:
##########
@@ -17,18 +17,16 @@
package org.apache.solr.metrics;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
import com.codahale.metrics.Clock;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.Timer;
import java.time.Duration;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class DelegateRegistryTimerTest {
+public class DelegateRegistryTimerTest extends SolrTestCase {
Review Comment:
Doubt
##########
solr/core/src/test/org/apache/solr/response/TestRetrieveFieldsOptimizer.java:
##########
@@ -616,7 +615,7 @@ List<String> getValsForField() {
break;
default:
- fail("Found no case for field " + name + " type " + type);
+ SolrTestCaseJ4.fail("Found no case for field " + name + " type " + type);
Review Comment:
Why to add class name?
##########
solr/prometheus-exporter/src/test/org/apache/solr/prometheus/exporter/MetricsQueryTemplateTest.java:
##########
@@ -36,7 +33,7 @@
import org.w3c.dom.Document;
import org.w3c.dom.NodeList;
-public class MetricsQueryTemplateTest {
+public class MetricsQueryTemplateTest extends SolrTestCaseJ4 {
Review Comment:
disagree
##########
solr/core/src/test/org/apache/solr/util/hll/NumberUtilTest.java:
##########
@@ -16,13 +16,11 @@
*/
package org.apache.solr.util.hll;
-import static org.junit.Assert.assertTrue;
-
-import java.util.Arrays;
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
/** Tests {@link NumberUtil} */
-public class NumberUtilTest {
+public class NumberUtilTest extends SolrTestCase {
Review Comment:
Disagree
##########
solr/solrj/src/test/org/apache/solr/client/solrj/request/SchemaTest.java:
##########
@@ -869,7 +869,7 @@ public void testCopyFieldWithMaxCharsAccuracy() throws Exception {
int currentMaxChars = (Integer) currentCopyField.get("maxChars");
MatcherAssert.assertThat(
currentDestFieldName, anyOf(is(equalTo(destFieldName1)), is(equalTo(destFieldName2))));
- assertTrue(maxChars == currentMaxChars);
+ assertEquals((int) maxChars, currentMaxChars);
Review Comment:
why (cast)?
##########
solr/solrj/src/test/org/apache/solr/common/util/URLUtilTest.java:
##########
@@ -16,13 +16,10 @@
*/
package org.apache.solr.common.util;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class URLUtilTest {
+public class URLUtilTest extends SolrTestCase {
Review Comment:
Why?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java:
##########
@@ -1075,15 +1075,15 @@ public void testGatherNodesFriendsStream() throws Exception {
tuples = getTuples(stream);
tuples.sort(new FieldComparator("node", ComparatorOrder.ASCENDING));
- assertTrue(tuples.size() == 4);
- assertTrue(tuples.get(0).getString("node").equals("bill"));
- assertTrue(tuples.get(0).getLong("level").equals(0L));
- assertTrue(tuples.get(1).getString("node").equals("jim"));
- assertTrue(tuples.get(1).getLong("level").equals(1L));
- assertTrue(tuples.get(2).getString("node").equals("max"));
- assertTrue(tuples.get(2).getLong("level").equals(1L));
- assertTrue(tuples.get(3).getString("node").equals("sam"));
- assertTrue(tuples.get(3).getLong("level").equals(1L));
+ assertEquals(4, tuples.size());
+ assertEquals("bill", tuples.get(0).getString("node"));
+ assertEquals(0L, (long) tuples.get(0).getLong("level"));
Review Comment:
How to avoid cast?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java:
##########
@@ -16,19 +16,16 @@
*/
package org.apache.solr.client.solrj.impl;
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-
import java.io.IOException;
import java.util.HashSet;
import java.util.Set;
+import org.apache.solr.SolrTestCase;
import org.apache.solr.client.solrj.ResponseParser;
import org.apache.solr.client.solrj.request.RequestWriter;
import org.junit.Test;
/** Test the LBHttp2SolrClient. */
-public class LBHttp2SolrClientTest {
+public class LBHttp2SolrClientTest extends SolrTestCase {
Review Comment:
Why?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/eval/ConversionEvaluatorsTest.java:
##########
@@ -32,7 +30,7 @@
import org.junit.Test;
/** Test ConversionEvaluators */
-public class ConversionEvaluatorsTest {
+public class ConversionEvaluatorsTest extends SolrTestCase {
Review Comment:
disagree
##########
solr/solrj/src/test/org/apache/solr/client/solrj/request/TestUpdateRequest.java:
##########
@@ -17,56 +17,55 @@
package org.apache.solr.client.solrj.request;
import java.util.Arrays;
+import org.apache.solr.SolrTestCase;
import org.apache.solr.common.SolrInputDocument;
-import org.junit.Assert;
import org.junit.Test;
-public class TestUpdateRequest {
+public class TestUpdateRequest extends SolrTestCase {
Review Comment:
Can't it be just TestCase or Assert?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/eval/TemporalEvaluatorsTest.java:
##########
@@ -54,7 +52,7 @@
import org.junit.Test;
/** Tests numeric Date/Time stream evaluators */
-public class TemporalEvaluatorsTest {
+public class TemporalEvaluatorsTest extends SolrTestCase {
Review Comment:
Changes below proves that adding ancestor is redundant
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/eval/ArrayEvaluatorTest.java:
##########
@@ -54,12 +53,12 @@ public void arrayLongSortAscTest() throws IOException {
result = evaluator.evaluate(new Tuple(values));
- Assert.assertTrue(result instanceof List<?>);
+ assertTrue(result instanceof List<?>);
- Assert.assertEquals(3, ((List<?>) result).size());
Review Comment:
can't it be casted once ?
##########
solr/modules/analytics/src/test/org/apache/solr/analytics/legacy/facet/LegacyFieldFacetCloudTest.java:
##########
@@ -442,48 +440,48 @@ public void sumTest() throws Exception {
String responseStr = response.toString();
// Int Date
- Collection<Double> intDate =
+ ArrayList<Double> intDate =
Review Comment:
can't it be `List<Double>` ?
##########
solr/core/src/test/org/apache/solr/update/processor/SkipExistingDocumentsProcessorFactoryTest.java:
##########
@@ -38,7 +36,7 @@
import org.junit.Test;
import org.mockito.Mockito;
-public class SkipExistingDocumentsProcessorFactoryTest {
+public class SkipExistingDocumentsProcessorFactoryTest extends SolrTestCaseJ4 {
Review Comment:
Disagree with just adding ancestor class.
##########
solr/modules/ltr/src/test/org/apache/solr/ltr/norm/TestStandardNormalizer.java:
##########
@@ -16,17 +16,14 @@
*/
package org.apache.solr.ltr.norm;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
import java.nio.file.Paths;
import java.util.HashMap;
import java.util.Map;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.core.SolrResourceLoader;
import org.junit.Test;
-public class TestStandardNormalizer {
+public class TestStandardNormalizer extends SolrTestCaseJ4 {
Review Comment:
disagree
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/expr/InjectionDefenseTest.java:
##########
@@ -17,12 +17,10 @@
package org.apache.solr.client.solrj.io.stream.expr;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class InjectionDefenseTest {
+public class InjectionDefenseTest extends SolrTestCase {
Review Comment:
Why?
##########
solr/prometheus-exporter/src/test/org/apache/solr/prometheus/collector/MetricSamplesTest.java:
##########
@@ -27,9 +25,10 @@
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class MetricSamplesTest {
+public class MetricSamplesTest extends SolrTestCase {
Review Comment:
Why?
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -2177,43 +2176,43 @@ public void testTimeSeriesGroupingFacet() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 6);
+ assertEquals(6, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("year_i") == 2015);
- assert (tuple.getLong("month_i") == 11);
- assert (tuple.getLong("day_i") == 8);
- assert (tuple.getDouble("EXPR$3") == 42); // sum(item_i)
+ assertEquals(2015, (long) tuple.getLong("year_i"));
Review Comment:
How to avoid (long) cast?
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -2308,43 +2308,43 @@ public void testParallelTimeSeriesGrouping() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 6);
+ assertEquals(6, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("year_i") == 2015);
- assert (tuple.getLong("month_i") == 11);
- assert (tuple.getLong("day_i") == 8);
- assert (tuple.getDouble("EXPR$3") == 42); // sum(item_i)
+ assertEquals(2015, (long) tuple.getLong("year_i"));
Review Comment:
`2015L` ?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/MathExpressionTest.java:
##########
@@ -1719,10 +1719,10 @@ public void testMatrix() throws Exception {
assertEquals(features.get(0).get(0), "col3");
assertEquals(features.get(1).get(0), "col1");
- assertTrue(tuples.get(0).getLong("f") == 2);
- assertTrue(tuples.get(0).getLong("g") == 3);
- assertTrue(tuples.get(0).getLong("h") == 1);
- assertTrue(tuples.get(0).getLong("i") == 2);
+ assertEquals(2, (long) tuples.get(0).getLong("f"));
Review Comment:
let me know if cast is avoidable
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998698318
##########
solr/core/src/test/org/apache/solr/handler/V2ClusterAPIMappingTest.java:
##########
@@ -48,7 +47,7 @@
import org.mockito.ArgumentCaptor;
/** Unit tests for the v2 to v1 API mappings found in {@link ClusterAPI} */
-public class V2ClusterAPIMappingTest {
+public class V2ClusterAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/core/src/test/org/apache/solr/handler/V2UpdateAPIMappingTest.java:
##########
@@ -39,7 +38,7 @@
import org.junit.Test;
/** Unit tests for the v2 to v1 mapping logic in {@link UpdateAPI} */
-public class V2UpdateAPIMappingTest {
+public class V2UpdateAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/core/src/test/org/apache/solr/handler/admin/SolrEnvironmentTest.java:
##########
@@ -17,13 +17,11 @@
package org.apache.solr.handler.admin;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-
+import org.apache.solr.SolrTestCase;
import org.apache.solr.common.SolrException;
import org.junit.Test;
-public class SolrEnvironmentTest {
+public class SolrEnvironmentTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/core/src/test/org/apache/solr/handler/admin/api/V2NodeAPIMappingTest.java:
##########
@@ -55,7 +53,7 @@
import org.mockito.ArgumentCaptor;
/** Unit tests for the v2 to v1 mapping for /node/ APIs. */
-public class V2NodeAPIMappingTest {
+public class V2NodeAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998697319
##########
solr/core/src/test/org/apache/solr/core/backup/BackupFilePathsTest.java:
##########
@@ -16,16 +16,13 @@
*/
package org.apache.solr.core.backup;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
import java.util.List;
import java.util.Optional;
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
/** Unit tests for {@link BackupFilePaths} */
-public class BackupFilePathsTest {
+public class BackupFilePathsTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:
##########
@@ -44,7 +39,7 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class BlobRepositoryMockingTest {
+public class BlobRepositoryMockingTest extends SolrTestCaseJ4 {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
--
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] mkhludnev commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
mkhludnev commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1284483387
@risdenk why do you think about `assertEquals(3, tuple.getLong("myId").longValue());`
Is it better than cast? Just asking. Overall, I'm ok with your changes.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998865176
##########
solr/core/src/test/org/apache/solr/handler/component/SpellCheckComponentTest.java:
##########
@@ -640,7 +640,7 @@ public void testThresholdTokenFrequency() throws Exception {
values = rsp.getValues();
spellCheck = (NamedList<?>) values.get("spellcheck");
suggestions = (NamedList<?>) spellCheck.get("suggestions");
- assertTrue(suggestions.get("suggestion") == null);
- assertTrue((Boolean) spellCheck.get("correctlySpelled") == false);
+ assertNull(suggestions.get("suggestion"));
+ assertFalse((boolean) (Boolean) spellCheck.get("correctlySpelled"));
Review Comment:
Fixed in c8f5620d11c39effa2edf94bb949c6d2413be6b0
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998697932
##########
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java:
##########
@@ -111,7 +108,7 @@ public static void assertVersions(SolrClient client1, SolrClient client2) throws
Long maxVersionClient2 = getVersion(client2);
if (maxVersionClient1 > 0 && maxVersionClient2 > 0) {
- assertEquals(maxVersionClient1, maxVersionClient2);
+ SolrTestCaseJ4.assertEquals(maxVersionClient1, maxVersionClient2);
Review Comment:
This is the JUnit one technically. It avoids the extra import and shows it comes through the underlying class heirarchy. LuceneTestCase extends Assert
##########
solr/core/src/test/org/apache/solr/handler/TestSampleDocumentsLoader.java:
##########
@@ -40,7 +35,7 @@
import org.junit.Before;
import org.junit.Test;
-public class TestSampleDocumentsLoader {
+public class TestSampleDocumentsLoader extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998698747
##########
solr/core/src/test/org/apache/solr/metrics/DelegateRegistryTimerTest.java:
##########
@@ -17,18 +17,16 @@
package org.apache.solr.metrics;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
import com.codahale.metrics.Clock;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.Timer;
import java.time.Duration;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class DelegateRegistryTimerTest {
+public class DelegateRegistryTimerTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
--
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] risdenk merged pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk merged PR #953:
URL: https://github.com/apache/solr/pull/953
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r999941029
##########
solr/core/src/test/org/apache/solr/core/QueryResultKeyTest.java:
##########
@@ -134,7 +134,6 @@ public void testRandomQueryKeyEquality() {
assertKeyNotEquals(aa, bb);
}
}
- assert minIters <= iter;
Review Comment:
This is always true.
##########
solr/core/src/test/org/apache/solr/search/join/BJQParserTest.java:
##########
@@ -178,7 +179,6 @@ public void testJustParentsFilterInChild() {
"//doc/arr[@name='child_s']/str='" + klm[0] + "'",
"//doc/arr[@name='child_s']/str='" + klm[1] + "'",
"//doc/arr[@name='child_s']/str='" + klm[2] + "'");
- assert klm.length == 3 : "change asserts pls " + Arrays.toString(klm);
Review Comment:
This is always true
##########
solr/core/src/test/org/apache/solr/legacy/TestLegacyFieldReuse.java:
##########
@@ -53,7 +53,6 @@ public void testNumericReuse() throws IOException {
// pass another bogus stream (numeric, but different precision step!)
legacyIntField = new LegacyIntField("foo", 42, Field.Store.NO);
- assert 3 != LegacyNumericUtils.PRECISION_STEP_DEFAULT;
Review Comment:
this is always true
##########
solr/core/src/test/org/apache/solr/handler/TestStressThreadBackup.java:
##########
@@ -243,8 +243,6 @@ public void run() {
// and how few iterations we have left
if (3 < namedSnapshots.size()
&& random().nextInt(3 + numBackupIters - i) < random().nextInt(namedSnapshots.size())) {
-
- assert 0 < namedSnapshots.size() : "Something broke the conditional";
Review Comment:
this is always true
--
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] madrob commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
madrob commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1203096343
@epugh apologies for the merge conflicts, I think some of those came up because of my big set of changes speeding up the tests.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998699192
##########
solr/core/src/test/org/apache/solr/parser/SolrQueryParserBaseTest.java:
##########
@@ -26,19 +24,17 @@
import java.util.List;
import org.apache.lucene.queryparser.charstream.CharStream;
import org.apache.lucene.search.Query;
+import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.common.SolrException;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.schema.IndexSchema;
import org.apache.solr.search.QParser;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
+import org.mockito.Mockito;
-@RunWith(MockitoJUnitRunner.class)
-public class SolrQueryParserBaseTest {
+public class SolrQueryParserBaseTest extends SolrTestCaseJ4 {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
This is still using mocks just instead of the MockitoJUnitRunner uses the Lucene runner. This also matches all other Mockito uses in the Solr project - this is the ONLY class using MockitoJUnitRunner
--
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] risdenk commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1285878072
one last merge of main. running tests again locally just to make sure. planning to merge this 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: 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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998701974
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttpSolrClientTest.java:
##########
@@ -16,17 +16,15 @@
*/
package org.apache.solr.client.solrj.impl;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-
import java.io.IOException;
import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.solr.SolrTestCase;
import org.apache.solr.client.solrj.ResponseParser;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.junit.Test;
/** Test the LBHttpSolrClient. */
-public class LBHttpSolrClientTest {
+public class LBHttpSolrClientTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientTest.java:
##########
@@ -17,22 +17,19 @@
package org.apache.solr.client.solrj.impl;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import org.apache.lucene.tests.util.LuceneTestCase;
+import org.apache.solr.SolrTestCase;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.request.QueryRequest;
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.junit.Test;
-public class LBSolrClientTest {
+public class LBSolrClientTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrPortAwareCookieSpecTest.java:
##########
@@ -21,11 +21,10 @@
import org.apache.http.cookie.MalformedCookieException;
import org.apache.http.impl.cookie.BasicClientCookie;
import org.apache.solr.SolrTestCaseJ4;
-import org.junit.Assert;
import org.junit.Test;
// Test cases imported from TestNetscapeCookieAttribHandlers of HttpClient project
-public class SolrPortAwareCookieSpecTest {
+public class SolrPortAwareCookieSpecTest extends SolrTestCaseJ4 {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
--
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] risdenk commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1283004310
So @mkhludnev and @HoustonPutman thanks for the reviews.
There are some benefits to using `SolrTestCase` as the base for these tests. The randomization of Locale, thread leak checks, and more. These checks are beneficial that we get some nice things without doing any extra work. SolrTestCase extends from Assert eventually so its easy to get all the static methods from there.
The question about SolrTestCase vs TestCase vs SolrTestCase4j - I used SolrTestCase if there was no existing import for one and SolrTestCase4j if there was already an existing import using SolrTestCase4j. My theory being that `SolrTestCase4j` already was needed so extend from it. I didn't see any downside to doing this.
Regarding the casting - I'll take a look and see what is possible here.
Regarding the assert methods using `SolrTestCase4j` or `SolrTestCase` - I was trying to avoid static imports but can be swayed if this is important to not do 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: 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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998771803
##########
solr/core/src/test/org/apache/solr/handler/TestSampleDocumentsLoader.java:
##########
@@ -63,7 +58,7 @@ public void testJson() throws Exception {
@Test
public void testCsv() throws Exception {
ModifiableSolrParams params = new ModifiableSolrParams();
- params.set(CSV_MULTI_VALUE_DELIM_PARAM, "\\|");
+ params.set(DefaultSampleDocumentsLoader.CSV_MULTI_VALUE_DELIM_PARAM, "\\|");
Review Comment:
yea this was me just removing the static imports. It makes it clear that this isn't a constant in the `TestSampleDocumentsLoader` class to me. Otherwise it looks like a field in the class.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998698569
##########
solr/core/src/test/org/apache/solr/handler/component/SpellCheckComponentTest.java:
##########
@@ -627,8 +627,8 @@ public void testThresholdTokenFrequency() throws Exception {
NamedList<?> values = rsp.getValues();
NamedList<?> spellCheck = (NamedList<?>) values.get("spellcheck");
NamedList<?> suggestions = (NamedList<?>) spellCheck.get("suggestions");
- assertTrue(suggestions.get("suggestion") == null);
- assertTrue((Boolean) spellCheck.get("correctlySpelled") == false);
+ assertNull(suggestions.get("suggestion"));
+ assertFalse((boolean) (Boolean) spellCheck.get("correctlySpelled"));
Review Comment:
agreed will fix.
##########
solr/core/src/test/org/apache/solr/handler/component/SpellCheckComponentTest.java:
##########
@@ -640,7 +640,7 @@ public void testThresholdTokenFrequency() throws Exception {
values = rsp.getValues();
spellCheck = (NamedList<?>) values.get("spellcheck");
suggestions = (NamedList<?>) spellCheck.get("suggestions");
- assertTrue(suggestions.get("suggestion") == null);
- assertTrue((Boolean) spellCheck.get("correctlySpelled") == false);
+ assertNull(suggestions.get("suggestion"));
+ assertFalse((boolean) (Boolean) spellCheck.get("correctlySpelled"));
Review Comment:
agreed will fix.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998866826
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -425,22 +424,22 @@ public void testBasicSelect() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 3);
+ assertEquals(3, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("myId") == 3);
- assert (tuple.getLong("myInt") == 20);
- assert (tuple.get("myString").equals("a"));
+ assertEquals(3, (long) tuple.getLong("myId"));
Review Comment:
Hmmm so `tuple.getLong` returns a `Long` because it also returns `null` sometimes. Changing to `long` can't be done for `tuple.getLong` since then `null` can't be returned. `3L` is still a `long` and can't be compared with `Long` apparently. I could change it to
```
assertEquals(Long.valueOf(3), tuple.getLong("myId"))
```
but that seems worse.
I don't have any other ideas right now :/
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -1915,17 +1914,17 @@ public void testTimeSeriesGrouping() throws Exception {
List<Tuple> tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 2);
+ assertEquals(2, tuples.size());
Tuple tuple;
tuple = tuples.get(0);
- assert (tuple.getLong("year_i") == 2015);
- assert (tuple.getDouble("EXPR$1") == 66); // sum(item_i)
+ assertEquals(2015, (long) tuple.getLong("year_i"));
+ assertEquals(66, tuple.getDouble("EXPR$1"), 0.0); // sum(item_i)
tuple = tuples.get(1);
- assert (tuple.getLong("year_i") == 2014);
- assert (tuple.getDouble("EXPR$1") == 7); // sum(item_i)
+ assertEquals(2014, (long) tuple.getLong("year_i"));
Review Comment:
Hmmm so `tuple.getLong` returns a `Long` because it also returns `null` sometimes. Changing to `long` can't be done for `tuple.getLong` since then `null` can't be returned. `3L` is still a `long` and can't be compared with `Long` apparently. I could change it to
```
assertEquals(Long.valueOf(3), tuple.getLong("myId"))
```
but that seems worse.
I don't have any other ideas right now :/
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998865282
##########
solr/core/src/test/org/apache/solr/handler/component/SpellCheckComponentTest.java:
##########
@@ -627,8 +627,8 @@ public void testThresholdTokenFrequency() throws Exception {
NamedList<?> values = rsp.getValues();
NamedList<?> spellCheck = (NamedList<?>) values.get("spellcheck");
NamedList<?> suggestions = (NamedList<?>) spellCheck.get("suggestions");
- assertTrue(suggestions.get("suggestion") == null);
- assertTrue((Boolean) spellCheck.get("correctlySpelled") == false);
+ assertNull(suggestions.get("suggestion"));
+ assertFalse((boolean) (Boolean) spellCheck.get("correctlySpelled"));
Review Comment:
Fixed in c8f5620d11c39effa2edf94bb949c6d2413be6b0
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998873683
##########
solr/solrj/src/test/org/apache/solr/client/solrj/request/SchemaTest.java:
##########
@@ -869,7 +869,7 @@ public void testCopyFieldWithMaxCharsAccuracy() throws Exception {
int currentMaxChars = (Integer) currentCopyField.get("maxChars");
MatcherAssert.assertThat(
currentDestFieldName, anyOf(is(equalTo(destFieldName1)), is(equalTo(destFieldName2))));
- assertTrue(maxChars == currentMaxChars);
+ assertEquals((int) maxChars, currentMaxChars);
Review Comment:
Fixed in d59d4e9075a88dd6c44a9890e70ce00dfcac4ca7
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r999816766
##########
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java:
##########
@@ -202,15 +199,16 @@ public static NamedList<Object> getDetails(SolrClient s) throws Exception {
@SuppressWarnings("unchecked")
NamedList<Object> details = (NamedList<Object>) res.get("details");
- assertNotNull("null details", details);
+ SolrTestCaseJ4.assertNotNull("null details", details);
return details;
}
public static void assertReplicationResponseSucceeded(NamedList<?> response) {
- assertNotNull("null response from server", response);
- assertNotNull("Expected replication response to have 'status' field", response.get("status"));
- assertEquals("OK", response.get("status"));
+ SolrTestCaseJ4.assertNotNull("null response from server", response);
Review Comment:
So the way to fix this is to go back to static import for this. I have no strong opinion either way on this. I'll leave it up to @mkhludnev and @HoustonPutman since I think both of you asked about it.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998771457
##########
solr/solrj/src/test/org/apache/solr/client/solrj/request/SchemaTest.java:
##########
@@ -869,7 +869,7 @@ public void testCopyFieldWithMaxCharsAccuracy() throws Exception {
int currentMaxChars = (Integer) currentCopyField.get("maxChars");
MatcherAssert.assertThat(
currentDestFieldName, anyOf(is(equalTo(destFieldName1)), is(equalTo(destFieldName2))));
- assertTrue(maxChars == currentMaxChars);
+ assertEquals((int) maxChars, currentMaxChars);
Review Comment:
I'll take a look
--
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] risdenk commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1284528274
6492b9a68252f92db11fbee074acbbbf05680a51 fixes a bunch of casts and some raw `assert` lines that I found along the way in the stream/sql test files that I was looking at.
--
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] risdenk commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1282473042
```
BUILD SUCCESSFUL in 21m 57s
585 actionable tasks: 534 executed, 51 up-to-date
```
🚀
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998701805
##########
solr/prometheus-exporter/src/test/org/apache/solr/prometheus/collector/MetricSamplesTest.java:
##########
@@ -27,9 +25,10 @@
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class MetricSamplesTest {
+public class MetricSamplesTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/prometheus-exporter/src/test/org/apache/solr/prometheus/exporter/MetricsQueryTemplateTest.java:
##########
@@ -36,7 +33,7 @@
import org.w3c.dom.Document;
import org.w3c.dom.NodeList;
-public class MetricsQueryTemplateTest {
+public class MetricsQueryTemplateTest extends SolrTestCaseJ4 {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java:
##########
@@ -16,19 +16,16 @@
*/
package org.apache.solr.client.solrj.impl;
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-
import java.io.IOException;
import java.util.HashSet;
import java.util.Set;
+import org.apache.solr.SolrTestCase;
import org.apache.solr.client.solrj.ResponseParser;
import org.apache.solr.client.solrj.request.RequestWriter;
import org.junit.Test;
/** Test the LBHttp2SolrClient. */
-public class LBHttp2SolrClientTest {
+public class LBHttp2SolrClientTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998700173
##########
solr/modules/langid/src/test/org/apache/solr/update/processor/SolrInputDocumentReaderTest.java:
##########
@@ -16,21 +16,19 @@
*/
package org.apache.solr.update.processor;
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
import java.util.Arrays;
+import org.apache.solr.SolrTestCase;
import org.apache.solr.common.SolrInputDocument;
import org.junit.Before;
import org.junit.Test;
-public class SolrInputDocumentReaderTest {
+public class SolrInputDocumentReaderTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/modules/analytics/src/test/org/apache/solr/analytics/legacy/facet/LegacyFieldFacetCloudTest.java:
##########
@@ -442,48 +440,48 @@ public void sumTest() throws Exception {
String responseStr = response.toString();
// Int Date
- Collection<Double> intDate =
+ ArrayList<Double> intDate =
Review Comment:
Probably, but the method actually returns an ArrayList itself. I didn't change the method response. The reason it can't be a collection is that is ambiguous when comparing assertEquals - errorprone catches 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: 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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998702196
##########
solr/solrj/src/test/org/apache/solr/common/util/URLUtilTest.java:
##########
@@ -16,13 +16,10 @@
*/
package org.apache.solr.common.util;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class URLUtilTest {
+public class URLUtilTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/request/TestUpdateRequest.java:
##########
@@ -17,56 +17,55 @@
package org.apache.solr.client.solrj.request;
import java.util.Arrays;
+import org.apache.solr.SolrTestCase;
import org.apache.solr.common.SolrInputDocument;
-import org.junit.Assert;
import org.junit.Test;
-public class TestUpdateRequest {
+public class TestUpdateRequest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998772021
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -425,22 +424,22 @@ public void testBasicSelect() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 3);
+ assertEquals(3, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("myId") == 3);
- assert (tuple.getLong("myInt") == 20);
- assert (tuple.get("myString").equals("a"));
+ assertEquals(3, (long) tuple.getLong("myId"));
Review Comment:
I think so - I'll take a look
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -1915,17 +1914,17 @@ public void testTimeSeriesGrouping() throws Exception {
List<Tuple> tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 2);
+ assertEquals(2, tuples.size());
Tuple tuple;
tuple = tuples.get(0);
- assert (tuple.getLong("year_i") == 2015);
- assert (tuple.getDouble("EXPR$1") == 66); // sum(item_i)
+ assertEquals(2015, (long) tuple.getLong("year_i"));
+ assertEquals(66, tuple.getDouble("EXPR$1"), 0.0); // sum(item_i)
tuple = tuples.get(1);
- assert (tuple.getLong("year_i") == 2014);
- assert (tuple.getDouble("EXPR$1") == 7); // sum(item_i)
+ assertEquals(2014, (long) tuple.getLong("year_i"));
Review Comment:
I think so - I'll take a look
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -2177,43 +2176,43 @@ public void testTimeSeriesGroupingFacet() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 6);
+ assertEquals(6, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("year_i") == 2015);
- assert (tuple.getLong("month_i") == 11);
- assert (tuple.getLong("day_i") == 8);
- assert (tuple.getDouble("EXPR$3") == 42); // sum(item_i)
+ assertEquals(2015, (long) tuple.getLong("year_i"));
Review Comment:
I think so - I'll take a look
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998772357
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/eval/ArrayEvaluatorTest.java:
##########
@@ -54,12 +53,12 @@ public void arrayLongSortAscTest() throws IOException {
result = evaluator.evaluate(new Tuple(values));
- Assert.assertTrue(result instanceof List<?>);
+ assertTrue(result instanceof List<?>);
- Assert.assertEquals(3, ((List<?>) result).size());
Review Comment:
I'll take a look
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998703160
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/expr/InjectionDefenseTest.java:
##########
@@ -17,12 +17,10 @@
package org.apache.solr.client.solrj.io.stream.expr;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class InjectionDefenseTest {
+public class InjectionDefenseTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/eval/TemporalEvaluatorsTest.java:
##########
@@ -54,7 +52,7 @@
import org.junit.Test;
/** Tests numeric Date/Time stream evaluators */
-public class TemporalEvaluatorsTest {
+public class TemporalEvaluatorsTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/eval/ConversionEvaluatorsTest.java:
##########
@@ -32,7 +30,7 @@
import org.junit.Test;
/** Test ConversionEvaluators */
-public class ConversionEvaluatorsTest {
+public class ConversionEvaluatorsTest extends SolrTestCase {
Review Comment:
Get all the benefits of the underlying randomization and thread leak checks from Lucene. This ensures that all these tests have a common base. There are no real downsides to doing this either.
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r999897558
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -425,22 +424,22 @@ public void testBasicSelect() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 3);
+ assertEquals(3, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("myId") == 3);
- assert (tuple.getLong("myInt") == 20);
- assert (tuple.get("myString").equals("a"));
+ assertEquals(3, (long) tuple.getLong("myId"));
Review Comment:
addressed in 6492b9a68252f92db11fbee074acbbbf05680a51 for all the `getLong` and some other redundant casts I found along the way.
--
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 #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1203191898
Not an issue! It's the nature of any long lived PR that it has this. I'm waiting on the SolrJ ZK work to get done before moving forward on this. I'm expecting a bunch of merge conflicts ;-)
--
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] risdenk commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1285907045
```
./gradlew clean
...
./gradlew check -Pvalidation.errorprone=true
...
BUILD SUCCESSFUL in 22m 31s
586 actionable tasks: 576 executed, 10 up-to-date
```
--
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] risdenk commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1284493030
> `assertEquals(3, tuple.getLong("myId").longValue());`
> Is it better than cast? Just asking. Overall, I'm ok with your changes.
OOOO I like that. I didn't try that option. I'll make that change now.
--
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] risdenk commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1281800456
merged main and addressed remaining simplifications of asserts.
--
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 #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1281424613
Going to merge this after #947 gets in I think!
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998708897
##########
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java:
##########
@@ -202,15 +199,16 @@ public static NamedList<Object> getDetails(SolrClient s) throws Exception {
@SuppressWarnings("unchecked")
NamedList<Object> details = (NamedList<Object>) res.get("details");
- assertNotNull("null details", details);
+ SolrTestCaseJ4.assertNotNull("null details", details);
return details;
}
public static void assertReplicationResponseSucceeded(NamedList<?> response) {
- assertNotNull("null response from server", response);
- assertNotNull("Expected replication response to have 'status' field", response.get("status"));
- assertEquals("OK", response.get("status"));
+ SolrTestCaseJ4.assertNotNull("null response from server", response);
Review Comment:
I'll take a look to see what can be done here.
##########
solr/core/src/test/org/apache/solr/update/SoftAutoCommitTest.java:
##########
@@ -637,6 +636,6 @@ public void clear() {
}
public void assertSaneOffers() {
- assertEquals("Failure of MockEventListener" + fail.toString(), 0, fail.length());
+ SolrTestCaseJ4.assertEquals("Failure of MockEventListener" + fail.toString(), 0, fail.length());
Review Comment:
because this is in `MockEventListener` and doesn't extend anything with that method. This uses `SolrTestCaseJ4` since it is the correct class in the chain that was already imported. `SolrTestCaseJ4` ends up inheriting from Assert anyway.
##########
solr/core/src/test/org/apache/solr/handler/V2ClusterAPIMappingTest.java:
##########
@@ -48,7 +47,7 @@
import org.mockito.ArgumentCaptor;
/** Unit tests for the v2 to v1 API mappings found in {@link ClusterAPI} */
-public class V2ClusterAPIMappingTest {
+public class V2ClusterAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
`import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;` was there so this class was already relying on stuff from SolrTestCaseJ4 so used SolrTestCaseJ4.
##########
solr/core/src/test/org/apache/solr/response/TestRetrieveFieldsOptimizer.java:
##########
@@ -616,7 +615,7 @@ List<String> getValsForField() {
break;
default:
- fail("Found no case for field " + name + " type " + type);
+ SolrTestCaseJ4.fail("Found no case for field " + name + " type " + type);
Review Comment:
because this is in `RetrieveField` and doesn't extend anything with that method. This uses `SolrTestCaseJ4` since it is the correct class in the chain that was already imported. `SolrTestCaseJ4` ends up inheriting from Assert anyway.
##########
solr/core/src/test/org/apache/solr/handler/V2UpdateAPIMappingTest.java:
##########
@@ -39,7 +38,7 @@
import org.junit.Test;
/** Unit tests for the v2 to v1 mapping logic in {@link UpdateAPI} */
-public class V2UpdateAPIMappingTest {
+public class V2UpdateAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
`import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;` was there so this class was already relying on stuff from SolrTestCaseJ4 so used SolrTestCaseJ4.
##########
solr/core/src/test/org/apache/solr/handler/admin/api/V2NodeAPIMappingTest.java:
##########
@@ -55,7 +53,7 @@
import org.mockito.ArgumentCaptor;
/** Unit tests for the v2 to v1 mapping for /node/ APIs. */
-public class V2NodeAPIMappingTest {
+public class V2NodeAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
`import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;` was there so this class was already relying on stuff from SolrTestCaseJ4 so used SolrTestCaseJ4.
##########
solr/core/src/test/org/apache/solr/parser/SolrQueryParserBaseTest.java:
##########
@@ -26,19 +24,17 @@
import java.util.List;
import org.apache.lucene.queryparser.charstream.CharStream;
import org.apache.lucene.search.Query;
+import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.common.SolrException;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.schema.IndexSchema;
import org.apache.solr.search.QParser;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
+import org.mockito.Mockito;
-@RunWith(MockitoJUnitRunner.class)
-public class SolrQueryParserBaseTest {
+public class SolrQueryParserBaseTest extends SolrTestCaseJ4 {
Review Comment:
`import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;` was there so this class was already relying on stuff from SolrTestCaseJ4 so used SolrTestCaseJ4.
the `super.setUp()` is checked by Lucene to ensure that things are properly inherited if overrode.
--
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] HoustonPutman commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998493044
##########
solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:
##########
@@ -71,7 +66,8 @@ public static void beforeClass() {
}
@Before
- public void setUp() {
+ public void setUp() throws Exception {
+ super.setUp();
Review Comment:
Is this necessary if it isn't using anything from the parent class?
##########
solr/core/src/test/org/apache/solr/handler/V2UpdateAPIMappingTest.java:
##########
@@ -39,7 +38,7 @@
import org.junit.Test;
/** Unit tests for the v2 to v1 mapping logic in {@link UpdateAPI} */
-public class V2UpdateAPIMappingTest {
+public class V2UpdateAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
Why not `SolrTestCase`?
##########
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java:
##########
@@ -111,7 +108,7 @@ public static void assertVersions(SolrClient client1, SolrClient client2) throws
Long maxVersionClient2 = getVersion(client2);
if (maxVersionClient1 > 0 && maxVersionClient2 > 0) {
- assertEquals(maxVersionClient1, maxVersionClient2);
+ SolrTestCaseJ4.assertEquals(maxVersionClient1, maxVersionClient2);
Review Comment:
Why not just extend `SolrTestCase` like the other files?
##########
solr/core/src/test/org/apache/solr/handler/V2ClusterAPIMappingTest.java:
##########
@@ -48,7 +47,7 @@
import org.mockito.ArgumentCaptor;
/** Unit tests for the v2 to v1 API mappings found in {@link ClusterAPI} */
-public class V2ClusterAPIMappingTest {
+public class V2ClusterAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
Why not `SolrTestCase`?
##########
solr/core/src/test/org/apache/solr/handler/admin/api/V2NodeAPIMappingTest.java:
##########
@@ -55,7 +53,7 @@
import org.mockito.ArgumentCaptor;
/** Unit tests for the v2 to v1 mapping for /node/ APIs. */
-public class V2NodeAPIMappingTest {
+public class V2NodeAPIMappingTest extends SolrTestCaseJ4 {
Review Comment:
`SolrTestCase`
##########
solr/core/src/test/org/apache/solr/parser/SolrQueryParserBaseTest.java:
##########
@@ -26,19 +24,17 @@
import java.util.List;
import org.apache.lucene.queryparser.charstream.CharStream;
import org.apache.lucene.search.Query;
+import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.common.SolrException;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.schema.IndexSchema;
import org.apache.solr.search.QParser;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
+import org.mockito.Mockito;
-@RunWith(MockitoJUnitRunner.class)
-public class SolrQueryParserBaseTest {
+public class SolrQueryParserBaseTest extends SolrTestCaseJ4 {
Review Comment:
Can this be `SolrTestCase`? Not sure why we need to do `super.setup()` below...
--
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] risdenk commented on a diff in pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #953:
URL: https://github.com/apache/solr/pull/953#discussion_r998866952
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -2177,43 +2176,43 @@ public void testTimeSeriesGroupingFacet() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 6);
+ assertEquals(6, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("year_i") == 2015);
- assert (tuple.getLong("month_i") == 11);
- assert (tuple.getLong("day_i") == 8);
- assert (tuple.getDouble("EXPR$3") == 42); // sum(item_i)
+ assertEquals(2015, (long) tuple.getLong("year_i"));
Review Comment:
Hmmm so `tuple.getLong` returns a `Long` because it also returns `null` sometimes. Changing to `long` can't be done for `tuple.getLong` since then `null` can't be returned. `3L` is still a `long` and can't be compared with `Long` apparently. I could change it to
```
assertEquals(Long.valueOf(3), tuple.getLong("myId"))
```
but that seems worse.
I don't have any other ideas right now :/
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandler.java:
##########
@@ -2308,43 +2308,43 @@ public void testParallelTimeSeriesGrouping() throws Exception {
tuples = getTuples(sParams, baseUrl);
- assert (tuples.size() == 6);
+ assertEquals(6, tuples.size());
tuple = tuples.get(0);
- assert (tuple.getLong("year_i") == 2015);
- assert (tuple.getLong("month_i") == 11);
- assert (tuple.getLong("day_i") == 8);
- assert (tuple.getDouble("EXPR$3") == 42); // sum(item_i)
+ assertEquals(2015, (long) tuple.getLong("year_i"));
Review Comment:
Hmmm so `tuple.getLong` returns a `Long` because it also returns `null` sometimes. Changing to `long` can't be done for `tuple.getLong` since then `null` can't be returned. `3L` is still a `long` and can't be compared with `Long` apparently. I could change it to
```
assertEquals(Long.valueOf(3), tuple.getLong("myId"))
```
but that seems worse.
I don't have any other ideas right now :/
--
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] dsmiley commented on pull request #953: SOLR-16311: Simplify assert statement
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #953:
URL: https://github.com/apache/solr/pull/953#issuecomment-1283405032
I just want to say "thank you" for an unexpectedly thorough review by @mkhludnev on a PR that many of us saw and passed on (I did).
--
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