SOLR-16311: Simplify assert statement

[epugh]

trafficstars

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 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 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

[madrob]

@epugh apologies for the merge conflicts, I think some of those came up because of my big set of changes speeding up the tests.

[epugh]

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 ;-)

[epugh]

Going to merge this after #947 gets in I think!

[risdenk]

merged main and addressed remaining simplifications of asserts.

[risdenk]

BUILD SUCCESSFUL in 21m 57s
585 actionable tasks: 534 executed, 51 up-to-date

🚀

[risdenk]

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.

[dsmiley]

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).

[risdenk]

@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.

[mkhludnev]

@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.

[risdenk]

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.

[risdenk]

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.

[risdenk]

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.

[risdenk]

one last merge of main. running tests again locally just to make sure. planning to merge this today

[risdenk]

./gradlew clean
...
./gradlew check -Pvalidation.errorprone=true
...
BUILD SUCCESSFUL in 22m 31s
586 actionable tasks: 576 executed, 10 up-to-date