I left a couple of review comments on the PR. Thanks, Eduard! On Thu, Jun 10, 2021 at 2:42 AM Eduard Tudenhoefner <edu...@dremio.com> wrote:
> Yes we're definitely not talking about rewriting lots of test code. @Ryan > would you like to review/comment/approve the PR then if you think the > change is good going forward? > Thanks for your feedback. > > On Thu, Jun 10, 2021 at 1:36 AM Ryan Blue <b...@apache.org> wrote: > >> Thanks for the additional examples, Eduard. I do like the assertions here >> and I'm fine with the idea of gradually moving over to them. Looks like >> some of the CharSequence and collection comparators would definitely be >> easier to use than `assertEquals(ImmutableSet.of(...), actualSet)`. >> >> As long as we're not talking about rewriting lots of tests, then I think >> it makes sense to introduce this as a test dependency and people can use it >> where it makes sense. >> >> On Wed, Jun 9, 2021 at 12:12 AM Eduard Tudenhoefner <edu...@dremio.com> >> wrote: >> >>> 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 >>>> >>> >> >> -- >> Ryan Blue >> > -- Ryan Blue
