Re: [DISCUSS] Conventions on assertions to use in tests

[Francesco Guardiani]

Hi all.

I've worked on a little tool to try to automatize a bit the conversion of
assertions to assertj https://github.com/slinkydeveloper/assertj-migrator.
It's not perfect, but it's an attempt to unify a bit the test codebase.

We applied it to flink-table modules and it worked like a charm, although
it required a couple of more commits to improve the tool output. Here is
the merged PR: https://github.com/apache/flink/pull/19039

I'll try to find some time in future to do a bulk conversion of other
modules as well, but you can also use it yourself when doing PRs and
updating the tests, so you don't have to manually convert the tests
themselves.

I hope it helps,
FG


On Tue, Dec 14, 2021 at 10:00 AM Jing Ge  wrote:

> 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  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 
> > 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 
> > > 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 
> > > wrote:
> > > >
> > > > > As @Matthias Pohl  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  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
> 

Re: [DISCUSS] Conventions on assertions to use in tests

[Jing Ge]

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  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 
> 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 
> > 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 
> > wrote:
> > >
> > > > As @Matthias Pohl  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  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 <
> 

Re: [DISCUSS] Conventions on assertions to use in tests

[Marios Trivyzas]

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  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 
> 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 
> wrote:
> >
> > > As @Matthias Pohl  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  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 
> > > > 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 <
> > > 

Re: [DISCUSS] Conventions on assertions to use in tests

[Till Rohrmann]

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 
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  wrote:
>
> > As @Matthias Pohl  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  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  >
> > > 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 
> > > 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.

Re: [DISCUSS] Conventions on assertions to use in tests

[Matthias Pohl]

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

> As @Matthias Pohl  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  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 
> > >
> > > 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 
> > 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 
> > 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
> > > >>
> > > 

Re: [DISCUSS] Conventions on assertions to use in tests

[Marios Trivyzas]

As @Matthias Pohl  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 
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  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 
> >
> > 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 
> 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 
> 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  >
> > >> >
> > >> > 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

Re: [DISCUSS] Conventions on assertions to use in tests

[Francesco Guardiani]

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

Re: [DISCUSS] Conventions on assertions to use in tests

[Francesco Guardiani]

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

Re: [DISCUSS] Conventions on assertions to use in tests

[Matthias Pohl]

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

Re: [DISCUSS] Conventions on assertions to use in tests

[Marios Trivyzas]

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

Re: [DISCUSS] Conventions on assertions to use in tests

[David Morávek]

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

Re: [DISCUSS] Conventions on assertions to use in tests

[Till Rohrmann]

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

Re: [DISCUSS] Conventions on assertions to use in tests

[Francesco Guardiani]

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

Re: [DISCUSS] Conventions on assertions to use in tests

[Arvid Heise]

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  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  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 
> 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
>> >
>> >
>> > 
>> >
>> > Follow us @VervericaData
>> >
>> > --
>> >
>> > Join Flink Forward  - 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
>> >
>>
>

Re: [DISCUSS] Conventions on assertions to use in tests

[Till Rohrmann]

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  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  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
> >
> >
> > 
> >
> > Follow us @VervericaData
> >
> > --
> >
> > Join Flink Forward  - 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
> >
>

Re: [DISCUSS] Conventions on assertions to use in tests

[Ingo Bürk]

I agree on AssertJ being so much more expressive that I think it would be
worth switching. However, we should be careful not to solve the issue of
too many assertion frameworks by adding another one on top of everything
(but also realizing that we cannot easily migrate all tests in one go).

On Fri, Nov 12, 2021 at 3:40 PM David Anderson  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
> >
> >
> > 
> >
> > Follow us @VervericaData
> >
> > --
> >
> > Join Flink Forward  - 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
> >
>

Re: [DISCUSS] Conventions on assertions to use in tests

[David Anderson]

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
>
>
> 
>
> Follow us @VervericaData
>
> --
>
> Join Flink Forward  - 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
>

[DISCUSS] Conventions on assertions to use in tests

[Francesco Guardiani]

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




Follow us @VervericaData

--

Join Flink Forward  - 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