Agree. Clarifying once more what our preferred option is here, is a good idea. So, +1 for unification. I don't have a strong opinion on what framework to use. But we may want to add this at the end of the discussion to our documentation (e.g. [1] or maybe the PR description?) to make users aware of it and be able to provide a reference in case it comes up again (besides this ML thread). Or do we already have something like that somewhere in the docs where I missed it?
Matthias [1] https://cwiki.apache.org/confluence/display/FLINK/Best+Practices+and+Lessons+Learned On Wed, Nov 17, 2021 at 11:13 AM Marios Trivyzas <mat...@gmail.com> wrote: > 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
