I think this is a very good idea, Matthias. +1 for backporting the jassert changes to 1.14 and 1.13 if possible.
Cheers, Till On Fri, Dec 3, 2021 at 11:38 AM Matthias Pohl <matth...@ververica.com> wrote: > Currently, we only added the jassert to the master branch. I was wondering > whether we could backport the corresponding PR [1] to release-1.14 and > release-1.13, too. Otherwise, we would have to implement tests twice when > providing PRs with new tests that need to be backported: The jassert > version for master and a hamcrest (or any other available library) for the > backports. > > It's not really a bugfix. But it might help developers with their > backports. > > Matthias > > [1] https://github.com/apache/flink/pull/17871 > > On Thu, Nov 25, 2021 at 12:54 PM Marios Trivyzas <mat...@gmail.com> wrote: > > > As @Matthias Pohl <matth...@ververica.com> mentioned, I agree that no1 > is > > to end up with consistency > > regarding the assertions in our tests, but I also like how those > assertions > > shape up with the AssertJ approach. > > > > On Thu, Nov 25, 2021 at 9:38 AM Francesco Guardiani < > > france...@ververica.com> > > wrote: > > > > > This is the result of experimenting around creating custom assertions > for > > > Table API types > > > https://github.com/slinkydeveloper/flink/commit/ > > > d1ce37a62c2200b2c3008a9cc2cac91234222fd5[1]. I will PR it once the two > > PRs > > > in the > > > previous mail get merged > > > > > > On Monday, 22 November 2021 17:59:29 CET Francesco Guardiani wrote: > > > > 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+Lesso > > > > > ns+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 > > > > > > > > -- > > Marios >
