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 >
