I'm also +1 both for unification and specifically for assertJ. I think it covers a wide variety of assertions and as Francesco mentioned it's easily extensible, so that we can create custom assertions where needed, and avoid repeating test code.
On Tue, Nov 16, 2021 at 9:57 AM David Morávek <d...@apache.org> wrote: > I don't have any strong opinions on the asserting framework that we use, > but big +1 for the unification. > > Best, > D. > > On Tue, Nov 16, 2021 at 9:37 AM Till Rohrmann <trohrm...@apache.org> > wrote: > > > Using JUnit5 with assertJ is fine with me if the community agrees. Having > > guides for best practices would definitely help with the transition. > > > > Cheers, > > Till > > > > On Mon, Nov 15, 2021 at 5:34 PM Francesco Guardiani < > > france...@ververica.com> > > wrote: > > > > > > It is a bit unfortunate that we have tests that follow different > > > patterns. > > > This, however, is mainly due to organic growth. I think the community > > > started with Junit4, then we chose to use Hamcrest because of its > better > > > expressiveness. > > > > > > That is fine, I'm sorry if my mail felt like a rant :) > > > > > > > Personally, I don't have a strong preference for which testing tools > to > > > use. The important bit is that we agree as a community, then document > the > > > choice and finally stick to it. So before starting to use assertj, we > > > should probably align with the folks working on the Junit5 effort > first. > > > > > > As Arvid pointed out, using assertj might help the people working on > the > > > junit5 effort as well, since assertj works seamlessly with junit4, > junit5 > > > and even other java testing frameworks. > > > > > > > But I'm not sure if it's wise to change everything at once also > > > from the perspective of less active contributors. We may alleviate that > > > pain by providing good guides though. So maybe, we should also include > a > > > temporal dimension into the discussion. > > > > > > This is why I'm proposing a convention and not a rewrite of all the > tests > > > at once, that's unfeasible. As you sad, we can provide guides, like in > > our > > > contribution guides, explaining our assertion convention, that is use > > > assertj or whatever other library we want to use and how. So then we > can > > > ask contributors to use such assertion convention when they PR new > tests > > or > > > when they modify existing ones. Something like that: > > > > > > https://github.com/apache/flink-web/commit/87c572ccd4e0ae48eeff3eb15ad9847d302e659d > > > > > > On Fri, Nov 12, 2021 at 5:07 PM Arvid Heise <ar...@apache.org> wrote: > > > > > >> JUnit5 migration is currently mostly prepared. The rules are being > > >> migrated > > >> [1] and Hang and Qingsheng have migrated most tests in their branch > > afaik > > >> (Kudos to them!). > > >> > > >> Using assertj would make migration easier as it's independent of the > > JUnit > > >> version. But the same can be said about hamcrest, albeit less > > expressive. > > >> > > >> I'm personally in favor of assertj (disclaimer I contributed to the > > >> project > > >> a bit). But I'm not sure if it's wise to change everything at once > also > > >> from the perspective of less active contributors. We may alleviate > that > > >> pain by providing good guides though. So maybe, we should also > include a > > >> temporal dimension into the discussion. > > >> > > >> [1] https://github.com/apache/flink/pull/17556 > > >> > > >> On Fri, Nov 12, 2021 at 3:58 PM Till Rohrmann <trohrm...@apache.org> > > >> wrote: > > >> > > >> > Thanks for starting this discussion Francesco. I think there is a > lot > > of > > >> > value in consistency because it makes it a lot easier to navigate > and > > >> > contribute to the code base. The testing tools are definitely one > > >> important > > >> > aspect of consistency. > > >> > > > >> > It is a bit unfortunate that we have tests that follow different > > >> patterns. > > >> > This, however, is mainly due to organic growth. I think the > community > > >> > started with Junit4, then we chose to use Hamcrest because of its > > better > > >> > expressiveness. Most recently, there was an effort started that > aimed > > at > > >> > switching over to Junit5 [1, 2]. @Arvid Heise <ar...@apache.org> > > knows > > >> > more about the current status. > > >> > > > >> > Personally, I don't have a strong preference for which testing tools > > to > > >> > use. The important bit is that we agree as a community, then > document > > >> the > > >> > choice and finally stick to it. So before starting to use assertj, > we > > >> > should probably align with the folks working on the Junit5 effort > > first. > > >> > > > >> > [1] > https://lists.apache.org/thread/jsjvc2cqb91pyh47d4p6olk3c1vxqm3w > > >> > [2] > https://lists.apache.org/thread/d9y5tzcl8wpk6ozmf8575qfzww450jpk > > >> > > > >> > Cheers, > > >> > Till > > >> > > > >> > On Fri, Nov 12, 2021 at 3:41 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 > > >> >> > > > >> >> > > >> > > > >> > > > > > > -- Marios
