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

[Francesco Guardiani <fr...@ververica.com>]

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 <ji...@ververica.com> 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 <ma...@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 <tr...@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 <ma...@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 <ma...@gmail.com>
> > > wrote:
> > > >
> > > > > As @Matthias Pohl <ma...@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 <
> > > > > francesco@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 <ma...@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 <
> > > > matthias@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 <
> > > matriv@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 <
> > dmvk@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 <
> > > > > trohrmann@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 <
> > > > > > > >> > > francesco@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
> >
>