I agree on AssertJ being so much more expressive that I think it would be worth switching. However, we should be careful not to solve the issue of too many assertion frameworks by adding another one on top of everything (but also realizing that we cannot easily migrate all tests in one go).
On Fri, Nov 12, 2021 at 3:40 PM David Anderson <dander...@apache.org> wrote: > For what it's worth, I recently rewrote all of the tests in flink-training > to use assertj, removing a mixture of junit4 assertions and hamcrest in the > process. I chose assertj because I found it to be more expressive and made > the tests more readable. > > +1 from me > > David > > On Fri, Nov 12, 2021 at 10:03 AM Francesco Guardiani < > france...@ververica.com> wrote: > > > Hi all, > > > > I wonder If we have a convention of the testing tools (in particular > > assertions) to use in our tests. If not, are modules free to decide on a > > convention on their own? > > > > In case of table, we have a mixed bag of different assertions of all > kinds, > > sometimes mixed even in the same test: > > > > - Assertions from junit 4 > > - Assertions from junit 5 > > - Hamcrest > > - Some custom assertions classes (e.g. RowDataHarnessAssertor) > > - assert instructions > > > > The result is that most tests are very complicated to read and > understand, > > and we have a lot of copy pasted "assertion methods" all around our > > codebase. > > > > For table in particular, I propose to introduce assertj [1] and develop a > > couple of custom assertions [2] for the types we use most in our tests, > > e.g. Row, RowData, DataType, LogicalType, etc... For example: > > > > assertFalse(row.isNullAt(1)); > > assert row instanceof GenericRowData; > > assertEquals(row.getField(1), > TimestampData.ofEpochMillis(expectedMillis)); > > > > Could be: > > > > assertThat(row) > > .getField(1, TimestampData.class) > > .isEqualToEpochMillis(expectedMillis) > > > > We could have these in table-common so every part of the table stack can > > benefit from it. Of course we can't take all our tests and convert them > to > > the new assertions, but as a policy we can enforce to use the new > > assertions convention for every new test or for every test we modify in > > future PRs. > > > > What's your opinion about it? Do you agree to have such kind of policy of > > using the same assertions? If yes, do you like the idea of using assertj > to > > implement such policy? > > > > FG > > > > [1] A library for assertions https://assertj.github.io, already used by > > the > > pulsar connector > > [2] > https://assertj.github.io/doc/#assertj-core-custom-assertions-creation > > -- > > > > Francesco Guardiani | Software Engineer > > > > france...@ververica.com > > > > > > <https://www.ververica.com/> > > > > Follow us @VervericaData > > > > -- > > > > Join Flink Forward <https://flink-forward.org/> - The Apache Flink > > Conference > > > > Stream Processing | Event Driven | Real Time > > > > -- > > > > Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany > > > > -- > > > > Ververica GmbH > > > > Registered at Amtsgericht Charlottenburg: HRB 158244 B > > > > Managing Directors: Karl Anton Wehner, Holger Temme, Yip Park Tung Jason, > > Jinwei (Kevin) Zhang > > >
