Thanks guys for your feedback. The idea was to not replace existing code when introducing AssertJ as that would probably rather cause merge conflicts for a lot of people. The idea was rather to give people a (better) alternative when testing certain things, such as collections, exceptions, paths, URIs and so on. People can still use JUnit assertions if they want to, but at least there's an option to use other assertions if needed for cases that are more difficult to express/do with Junit assertions.
The PR was rather meant as an example to start the discussion and you're right, it mostly changes *assertEquals()* and that shouldn't be a good reason to switch to a different assertion lib. Unfortunately the PR doesn't show the full strengths and benefits of AssertJ, since I didn't want to adjust too many places (only one subproject and the AssertHelpers class). Also I probably wasn't very good at explaining why I believe AssertJ is a real benefit to the project, so let me try that here: - even though there is the *AssertHelpers* class, it's flexibility is rather limited to the given method signatures. If you want more flexibility when testing exceptions, you most likely would need to introduce a new helper method to achieve what you want. With AssertJ you have that flexibility if needed as can be seen in a few examples here <https://github.com/assertj/assertj-examples/blob/main/assertions-examples/src/test/java/org/assertj/examples/ExceptionAssertionsExamples.java#L38> - performing assertions on Streams <https://github.com/assertj/assertj-examples/blob/main/assertions-examples/src/test/java/org/assertj/examples/StreamAssertionsExamples.java#L51> / Collections <https://github.com/assertj/assertj-examples/blob/main/assertions-examples/src/test/java/org/assertj/examples/ListSpecificAssertionsExamples.java> / Iterables <https://github.com/assertj/assertj-examples/blob/main/assertions-examples/src/test/java/org/assertj/examples/IterableAssertionsExamples.java#L72> is a bit of a pain with plain Junit Assertions and the code rather ends up being much longer than needed, whereas the linked examples show that this can be achieved in a more concise and readable way - AssertJ also has some nice Path <https://github.com/assertj/assertj-examples/blob/main/assertions-examples/src/test/java/org/assertj/examples/PathAssertionsExamples.java#L43> assertions, which might be of benefit - performing assertions on Strings <https://github.com/assertj/assertj-examples/blob/main/assertions-examples/src/test/java/org/assertj/examples/StringAssertionsExamples.java#L32> is also a thing that's a bit more involved with plain JUnit - another thing is that when an assertion fails, AssertJ will generally provide more context and better error messages. This is a bit difficult to describe and I think one only sees the benefit when that actually happens. With JUnit assertions I have quite often found myself having to run a failing CI test locally first to understand why it's failing and what the actual & expected data was (think about Streams/Collections and such in terms of data) Here <https://github.com/assertj/assertj-examples/tree/main/assertions-examples/src/test/java/org/assertj/examples> are many more examples for people that are interested. I believe that the learning curve is very small for AssertJ, as most of the assertions start with *assertThat(...)* and then auto-completion suggests what can be done. On Tue, Jun 8, 2021 at 7:13 PM Ryan Blue <b...@apache.org> wrote: > I mostly agree with Jack. I think that if I were starting a new project, > I'd probably want to use the new assertions because they are readable, > appear to be type-specific, and have some nice helpers for some types. But > I don't see a lot of benefit to moving over to them right now and it would > be a significant amount of changes. The biggest advantage is already solved > by AssertHelpers, too. > > On Tue, Jun 8, 2021 at 9:27 AM Jack Ye <yezhao...@gmail.com> wrote: > >> I would be on the more cautious side when introducing a new test utils >> library. Based on the PR, we are mostly changing things like >> Assert.assertEquals to another syntax, but that syntax is not complex in >> the first place. If we introduce AssertJ, there will be a mixture of 2 >> syntaxes, which is confusing and also there is a learning curve for the new >> library. >> >> We already have AssertHelpers to deal with exceptions, and I don't see >> why we should have 2 ways to do the same thing. >> >> Are there any additional benefits that I didn't see? >> >> -Jack Ye >> >> >> On Tue, Jun 8, 2021 at 8:29 AM Eduard Tudenhoefner <edu...@dremio.com> >> wrote: >> >>> Hi everyone, >>> >>> I was wondering what the appetite would be for introducing AssertJ >>> <https://assertj.github.io/doc/> to the project? >>> I believe it's a really good testing library that makes writing >>> assertions much more intuitive, as the assertions are written in kind-of a >>> fluent way. The test code ends up being more readable and it provides an >>> actually useful error message when assertions fail. >>> >>> There are some good examples of how AssertJ is used here >>> <https://assertj.github.io/doc/#assertj-core-assertions-guide>, but >>> personally what I like most about AssertJ is testing exceptional code >>> <https://assertj.github.io/doc/#assertj-core-exception-assertions-assertThatThrownBy>, >>> where you want to make sure some code throws a particular exception and >>> also has message Xyz or some other property that you want to assert on (no >>> more *@Test(expected = SomeException.class)* or *try-catch *code with >>> Assert.fail()). >>> >>> I've seen that the project already has the *AssertHelpers* class, but I >>> believe we can improve some stuff there as well and make overall testing >>> nicer. >>> >>> I took the liberty and opened PR#2684 >>> <https://github.com/apache/iceberg/pull/2684>, which introduces AssertJ >>> to a subset of tests just to show its usage and its benefit. >>> >>> Please let me know what you think >>> >>> Eduard >>> >> > > -- > Ryan Blue >
