Thanks for bringing this up Jack. The reason for this is actually that I mistakenly applied "*withFailMessage()*" instead of "*as(..)*" in *AssertHelpers* and therefore the actual error reporting was overwritten. I opened PR#2706 <https://github.com/apache/iceberg/pull/2706> to fix this.
This is how it should actually look with the proposed fix in the PR: *[add a field of non-primitive type should fail]* *Expecting throwable message: "Cannot add field locations as an identifier field: not a primitive type field"to contain: "random bla ... not a primitive type field"but did not.* Throwable that failed the check: java.lang.IllegalArgumentException: Cannot add field locations as an identifier field: not a primitive type field at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:217) at org.apache.iceberg.SchemaUpdate.validateIdentifierField(SchemaUpdate.java:475) at org.apache.iceberg.SchemaUpdate.lambda$applyChanges$1(SchemaUpdate.java:467) at java.lang.Iterable.forEach(Iterable.java:75) at org.apache.iceberg.SchemaUpdate.applyChanges(SchemaUpdate.java:467) at org.apache.iceberg.SchemaUpdate.apply(SchemaUpdate.java:380) at org.apache.iceberg.SchemaUpdate.apply(SchemaUpdate.java:49) at org.apache.iceberg.TestSchemaUpdate.lambda$testSetIdentifierFieldsFails$38(TestSchemaUpdate.java:1343) at org.assertj.core.api.ThrowableAssert.catchThrowable(ThrowableAssert.java:62) at org.assertj.core.api.AssertionsForClassTypes.catchThrowable(AssertionsForClassTypes.java:877) at org.assertj.core.api.Assertions.catchThrowable(Assertions.java:1306) at org.assertj.core.api.Assertions.assertThatThrownBy(Assertions.java:1178) at org.apache.iceberg.AssertHelpers.assertThrows(AssertHelpers.java:43) at org.apache.iceberg.TestSchemaUpdate.testSetIdentifierFieldsFails(TestSchemaUpdate.java:1338) On Thu, Jun 17, 2021 at 2:31 AM Jack Ye <yezhao...@gmail.com> wrote: > 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 >> >
