Thanks! This looks good to me! -Jack Ye On Thu, Jun 17, 2021 at 12:14 AM Eduard Tudenhoefner <edu...@dremio.com> wrote:
> 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 >>> >>
