Hi all, Given I see generally consensus around having a convention and using assertj, I propose to merge these 2 PRs:
* Add the explanation of this convention in our code quality guide: https://github.com/apache/flink-web/pull/482 * Add assertj to dependency management in the parent pom and link in the PR template the code quality guide: https://github.com/apache/flink/pull/17871 WDYT? Once we merge those, I'll work in the next days to add some custom assertions in table-common for RowData and Row (commonly asserted everywhere in the table codebase). @Matthias Pohl <matth...@ververica.com> about the confluence page, it seems a bit outdated, judging from the last modified date. I propose to continue to use this guide https://flink.apache.org/contributing/code-style-and-quality-common.html as it seems more complete. On Mon, Nov 22, 2021 at 8:58 AM Matthias Pohl <matth...@ververica.com> wrote: > 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 > >
