Definitely +1 from me as well. Otherwise backporting tests (accompanying fixes) would consume significant time.
On Fri, Dec 3, 2021 at 11:42 AM Till Rohrmann <trohrm...@apache.org> wrote: > 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 > > > -- Marios
