-
Notifications
You must be signed in to change notification settings - Fork 664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SOLR-16300: migrate assert to non deprecated version #947
Conversation
Replace junit inherited assertThat with explicit MatcherAssert class
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java
Outdated
Show resolved
Hide resolved
Did a bit more work, and I think we should:
|
So, I am rethinking this PR... I think I was falling into the trap of having too many different changes in one PR... So, I'm going to remove the task "Double check all the files for other asserts that can be simplified.", as that is a LOT of changes. I will go through and clean up any |
@cpoerschke I'd love a review, I think this is ready! |
See also apache/lucene#1049 and LUCENE-10662 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
19 / 26 files viewed
I wonder if we could/should also put the assert versions in question on the 'forbidden list' e.g. https://github.com/apache/solr/tree/main/gradle/validation/forbidden-apis or in some other way to avoid re-introduction of what is cleaned up here?
That is a great suggestion. I haven't touched that feature of our build system before. |
You have 3 options:
I'd add a separate txt file with correct name. The Gradle build automatically loads some forbiddenapis files when a specific dependency is in classpath. So it can be names forbiddenapis-hamcrest.txt (just example, the file name semantics are described in forbiddenapis.gradle). This prevents us from applying forbiddenapis in modules where the forbidden JAR file is not on classpath. |
I gave it a stab of adding it to forbiddenapis, but didn't quite get it to work... There are some places that it's okay, like in the LTR module... May need a pairing session to grok this! |
We can delay this to a separate PR. I also need try and error for this, because @dweiss implemented that Gradle code which picks the signatures files from that folder. |
I tried the forbiddenapis, and I think I didn't quite grok how it all worked, and ran out of energy.... If you wanted to add them to forbiddenapis, that would be great.. Otherwise, I'm happy to see this get merged!!! Thanks for reviewing and updating these older PR's!!! |
I added forbiddenapis and went down the rabbit hole w/ this. So we can't override assertThat in SolrTestCase since its static and comes from Assert which is what LuceneTestCase derives from. ForbiddenApis correctly complains that we can't just override the static method and instead knows that Assert#assertThat is still being called. So the solution in this PR isn't going to work as is. The MatcherAssert.assertThat static import also doesn't work in each file since the base class already has the static method assertThat - so the only solution is to explicitly do MatcherAssert.assertThat. I have a change that does that that I'll push shortly including the forbiddenapis check. |
it this PR maybe just going down the wrong approach? I'm not wedded to this one, it felt a bit werid to me as well. |
no I don't think so. As Uwe said, should really be Class#method for static and this now follows that approach. Ideally this would have been the same way as Assert.assertThat originally, but oh well. This at least makes everything consistent. It only looks ugly once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go. @epugh @cpoerschke @uschindler can you take another look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve a PR I started, but I ran gw check
and it looks great. This is a huge improvement in getting rid of old deprecated code compared to where I left it, so excited to get this in! What will we do with all our time if we aren't updating deprecated code in Solr ;-).
I'm getting a bunch of failures...
so need to fix these:
|
I was on the wrong branch, and just was testing out
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
./gradlew check
looks good to me now :)
Okay, this time things ran! Thanks for the fixes @risdenk |
Co-authored-by: Kevin Risden <krisden@apache.org>
https://issues.apache.org/jira/browse/SOLR-16300
Description
intellij has flagged various deprecations in how we assert in our tests.
Solution
This PR has two approaches for migrating. One is to use the non deprecated class MatcherAssert, the other is to override the deprecated method from the
org.junit.Assert
class that our tests inherit from, and use the non deprecated class.Tests
Ran
gw test
successfully.