It came in as part of https://github.com/apache/solr/pull/947#issuecomment-1279651282
I linked to one of the comments there since this was discussed on the PR. Basically it removes a ton of deprecated usages. If/when we upgrade Junit we should hopefully have much less to do. So to answer your question > - What is so terrible about the impl of Assert.assertThat? > Nothing just that it is deprecated and trying to stay on top of avoiding the use of deprecated code if we can. - What evil does it contain that makes it completely unssuitable for use? > Again same as above since it is the same question twice. I'll give you that its not ideal, but we have to draw the line somewhere. Kevin Risden On Wed, Oct 25, 2023 at 4:27 PM Chris Hostetter <hossman_luc...@fucit.org> wrote: > > Begin Rant... > > $ tail -6 ./gradle/validation/forbidden-apis/junit.junit.all.txt > junit.framework.TestCase @ All classes should derive from LuceneTestCase > junit.framework.Assert @ Use org.junit.Assert > junit.framework.** @ Use org.junit > org.junit.Assert#assertThat(**) @ Use > org.hamcrest.MatcherAssert.assertThat() > org.junit.matchers.JUnitMatchers @ Use org.hamcrest.CoreMatchers > org.junit.rules.ExpectedException @ Use Assert.assertThrows > > So all test classes should: > - derive from LuceneTestCase (agreed) > - use org.junit.Assert instead of junit.framework.Assert (fair enough) > - use MatcherAssert.assertThat() instead of Assert.assertThat() ... > > ...ok, i guess? ... except that LuceneTestCase extends Assert, giving us > Assert.assertThat() for free. Kind of anoying to have to go out of our > way to import another class to do the same thing. > > But also ... you can't just use... > > import static org.hamcrest.MatcherAssert.assertThat; > > ...because (assuming you're following best practices) you're already > inheriting from LuceneTestCase, so that static import is going to be > unused since it conflicts with the inherited 'assertThat' -- or at least > that's what ecjLintTest tells you ... i'm not 100% certain it's correct, > but either way you can't use it because it will fails the build. > > So your only recourse is to use... > > import org.hamcrest.MatcherAssert; > > ...and now instead of lots of nice short 'assertThat(...)' method calls > you have to use the twice as long 'MatcherAssert.assertThat', which is > probably going to make spotless decide your assertions needs to span at > least one extra line (assuming you are using descriptive variable names) > > All because we don't want want devs to use a (deprecated) method > that's indirectly inherited from an ancestor of a baseclass we tell them > to use. > > Which begs the questions: > > - What is so terrible about the impl of Assert.assertThat? > - What evil does it contain that makes it completely unssuitable for use? > > Let's go find out what exactly we are asking forbidden-apis to > shield us from... > > > https://github.com/junit-team/junit4/blob/r4.13.2/src/main/java/org/junit/Assert.java#L962 > > public static <T> void assertThat(String reason, T actual, > Matcher<? super T> matcher) { > MatcherAssert.assertThat(reason, actual, matcher); > } > > ...oh dear lord ... what sweet horror is this??!?!?!!? > > Thank heavens we were wise enough to protect ourselves from this > unspeakably dangerous code! > > > > -Hoss > http://www.lucidworks.com/ > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@solr.apache.org > For additional commands, e-mail: dev-h...@solr.apache.org > >
