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 > >> >> > > >> >> > >> > > >> > > >
