After submitting a few new PRs recently, I propose that we revert the changes made in the PR to use AssertJ in AssertHelpers.
The change made the error message very unclear for what was causing the assertion to fail. For example, if I change the expected error message in a test, the current junit error using assertj is: add a field with name not exist should fail java.lang.AssertionError: add a field with name not exist should fail at org.apache.iceberg.AssertHelpers.assertThrows(AssertHelpers.java:47) at org.apache.iceberg.TestSchemaUpdate.testSetIdentifierFieldsFails(TestSchemaUpdate.java:1337) Which does not help debugging at all, whereas in the past we get a diff between the output and the expected error message, so that we know what is actually happening in the call block: Expected exception message (not found in current schemas or added columns) missing: Cannot add field %s as an identifier field: not found in current schema or added columns java.lang.AssertionError: Expected exception message (not found in current schemas or added columns) missing: Cannot add field %s as an identifier field: not found in current schema or added columns at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.assertTrue(Assert.java:41) at org.apache.iceberg.AssertHelpers.handleException(AssertHelpers.java:129) at org.apache.iceberg.AssertHelpers.assertThrows(AssertHelpers.java:47) at org.apache.iceberg.TestSchemaUpdate.testSetIdentifierFieldsFails(TestSchemaUpdate.java:1337) We can keep AssertJ in the test library so people can use it if necessary, but for AssertHelpers it seems to be less effective than before. -Jack Ye On Thu, Jun 10, 2021 at 10:07 AM Ryan Blue <b...@apache.org> wrote: > 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 >
