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
