If it encourages more and higher quality test writing +1 (nb). Also, low risk given it’s a test dep.
Using QuickTheories as an example, merging it with a new or updated test could be a good way to get it merged Jordan On Tue, Mar 10, 2020 at 10:33 AM Benjamin Lerer <benjamin.le...@datastax.com> wrote: > +1 > > On Tue, Mar 10, 2020 at 6:18 PM Jon Haddad <j...@jonhaddad.com> wrote: > > > I've used assertj in a lot of projects, I prefer it by a wide margin over > > using only junit. > > > > On Tue, Mar 10, 2020 at 9:45 AM David Capwell <dcapw...@gmail.com> > wrote: > > > > > +1 from me > > > > > > In CASSANDRA-15564 I build my own assert chain to make the tests > cleaner; > > > did it since assertj wasn't there. > > > > > > On Tue, Mar 10, 2020, 9:28 AM Kevin Gallardo < > > kevin.galla...@datastax.com> > > > wrote: > > > > > > > I would like to propose adding AssertJ < > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__assertj.github.io_doc_&d=DwIFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=Jad7nE1Oab1mebx31r7AOfSsa0by8th6tCxpykmmOBA&m=WrkWi_LeOnAl7rqft1DM27OEkXD7sc2fnZMy_-c7IS8&s=D4FAaGRQi2WlwKAOQbQMfyt_cRqsOuZdePUDgchdLhA&e= > > > > > > as > > > > a test dependency and therefore have it available for writing > > > > unit/distributed/any test assertions. > > > > > > > > In addition to the examples mentioned on the AssertJ docs page > (allows > > to > > > > do elaborate and comprehensible assertions on Collections, String, > and > > > > *custom > > > > assertions*), here's an example of a dtest I was looking at, that > could > > > be > > > > translated to AssertJ syntax, just to give an idea of how the syntax > > > would > > > > apply: > > > > > > > > *JUnit asserts*: > > > > try { > > > > [...] > > > > } catch (Exception e) { > > > > Assert.assertTrue(e instanceof RuntimeException); > > > > RuntimeException re = ((RuntimeException) e); > > > > Assert.assertTrue(re.getCause() instanceof ReadTimeoutException); > > > > ReadTimeoutException rte = ((ReadTimeoutException) e.getCause()); > > > > Assert.assertTrue(rte.getMessage().contains("blabla") > > > > && rte.getMessage().contains("andblablo")); > > > > } > > > > > > > > *AssertJ style:* > > > > try { > > > > [...] > > > > } catch (Exception e) { > > > > Assertions.assertThat(e) > > > > .isInstanceOf(RuntimeException.class) > > > > .hasCauseExactlyInstanceOf(ReadTimeoutException.class) > > > > .hasMessageContaining("blabla") > > > > .hasMessageContaining("andblablo"); > > > > } > > > > > > > > The syntax is more explicit and more comprehensible, but more > > > importantly, > > > > when one of the JUnit assertTrue() fails, you don't know *why*, you > > only > > > > know that the resulting boolean expression is false. > > > > If a failure happened with the assertJ tests, the failure would say > > > > "Exception > > > > did not contain expected message, expected "blabla", actual > > "notblabla"" > > > > (same for a lot of other situations), this makes debugging a failure, > > > after > > > > a test ran and failed much easier. With JUnit asserts you would have > to > > > > additionally add a message explaining what the expected value is > *and* > > > > what the > > > > actual value is, for each assert that is more complex than a > > assertEquals > > > > on a number, I suppose. I have seen a lot of tests so far that only > > test > > > > the expected behavior via assertTrue and does not show the incorrect > > > values > > > > when the test fails, which would come for free with AssertJ. > > > > > > > > Other examples randomly picked from the test suite: > > > > > > > > > > > > > > > > > > *org.apache.cassandra.repair.RepairJobTest#testNoTreeRetainedAfterDistance:* > > > > Replace assertion: > > > > assertTrue(messages.stream().allMatch(m -> m.verb() == > Verb.SYNC_REQ)); > > > > With: > > > > assertThat(messages) > > > > .extracting(Message::verb) > > > > .containsOnly(Verb.SYNC_REQ); > > > > > > > > As a result, if any of the messages is not a Verb.SYNC_REQ, the test > > > > failure will show the actual "Verb"s of messages. > > > > > > > > Replace: > > > > assertTrue(millisUntilFreed < TEST_TIMEOUT_S * 1000); > > > > With: > > > > assertThat(millisUntilFreed) > > > > .isLessThan(TEST_TIMEOUT_S * 1000); > > > > > > > > Same effect if the condition is not satisfied, more explicit error > > > message > > > > explaining why the test failed. > > > > > > > > AssertJ also allows Custom assertions which are also very useful and > > > could > > > > potentially be leveraged in the future. > > > > > > > > This would only touch on the tests' assertions, the rest of the test > > > setup > > > > and execution remains untouched (still uses JUnit for the test > > > execution). > > > > > > > > Thanks. > > > > > > > > -- > > > > Kévin Gallardo. > > > > > > > > > >
