Hi all, I took a close look at assertj and found there are two concepts for writing tests with two entry points interfaces: WithAssertions for normal style and BDDAssertions for BDD style. I would not suggest using them in one project simultaneously. Since all related work done previously were using the normal style afaik, the normal style seems to be the right one to stick with.
WDYT? Best regards Jing On Fri, Dec 3, 2021 at 12:15 PM Marios Trivyzas <mat...@gmail.com> wrote: > 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 >
