You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flink.apache.org by Panagiotis Garefalakis <pg...@apache.org> on 2023/05/01 05:57:01 UTC
Re: [DISCUSS] Preventing Mockito usage for the new code with Checkstyle
Hey Jing,
That's basically up to us to decide -- maybe a continuous approach is
safer? (tests with junit4 behaviour won't be mergeable if we enable rules
immediately)
That said, looks like the migration work is almost there
<https://issues.apache.org/jira/browse/FLINK-25325> -- with some effort we
could minimize the need for suppressions altogether.
Cheers,
Panagiotis
On Sun, Apr 30, 2023 at 6:16 AM Jing Ge <ji...@ververica.com.invalid> wrote:
> Thanks @Panagiotis for the hint! Does it mean that those suppressions need
> to be cleaned up continuously while we move forward with Junit5
> migration(extra guideline is required), even if regex has been used? Or we
> just leave them as they are and clean them up in one shot after every
> junit4 has been migrated to junit5.
>
> Best regards,
> Jing
>
> On Wed, Apr 26, 2023 at 9:02 AM Panagiotis Garefalakis <pg...@apache.org>
> wrote:
>
> > Thanks for bringing this up! +1 for the proposal
> >
> > @Jing Ge -- we don't necessarily need to completely migrate to Junit5
> (even
> > though it would be ideal).
> > We could introduce the checkstyle rule and add suppressions for the
> > existing problematic paths (as we do today for other rules e.g.,
> > AvoidStarImport)
> >
> > Cheers,
> > Panagiotis
> >
> > On Tue, Apr 25, 2023 at 11:48 PM Weihua Hu <hu...@gmail.com>
> wrote:
> >
> > > Thanks for driving this.
> > >
> > > +1 for Mockito and Junit4.
> > >
> > > A clarity checkstyle will be of great help to new developers.
> > >
> > > Best,
> > > Weihua
> > >
> > >
> > > On Wed, Apr 26, 2023 at 1:47 PM Jing Ge <ji...@ververica.com.invalid>
> > > wrote:
> > >
> > > > This is a great idea, thanks for bringing this up. +1
> > > >
> > > > Also +1 for Junit4. If I am not mistaken, it could only be done after
> > the
> > > > Junit5 migration is done.
> > > >
> > > > @Chesnay thanks for the hint. Do we have any doc about it? If not, it
> > > might
> > > > deserve one. WDYT?
> > > >
> > > > Best regards,
> > > > Jing
> > > >
> > > > On Wed, Apr 26, 2023 at 5:13 AM Lijie Wang <wangdachui9501@gmail.com
> >
> > > > wrote:
> > > >
> > > > > Thanks for driving this. +1 for the proposal.
> > > > >
> > > > > Can we also prevent Junit4 usage in new code by this way？Because
> > > > currently
> > > > > we are aiming to migrate our codebase to JUnit 5.
> > > > >
> > > > > Best,
> > > > > Lijie
> > > > >
> > > > > Piotr Nowojski <pn...@apache.org> 于2023年4月25日周二 23:02写道：
> > > > >
> > > > > > Ok, thanks for the clarification.
> > > > > >
> > > > > > Piotrek
> > > > > >
> > > > > > wt., 25 kwi 2023 o 16:38 Chesnay Schepler <ch...@apache.org>
> > > > > napisał(a):
> > > > > >
> > > > > > > The checkstyle rule would just ban certain imports.
> > > > > > > We'd add exclusions for all existing usages as we did when
> > > > introducing
> > > > > > > other rules.
> > > > > > > So far we usually disabled checkstyle rules for a specific
> files.
> > > > > > >
> > > > > > > On 25/04/2023 16:34, Piotr Nowojski wrote:
> > > > > > > > +1 to the idea.
> > > > > > > >
> > > > > > > > How would this checkstyle rule work? Are you suggesting to
> > start
> > > > > with a
> > > > > > > > number of exclusions? On what level will those exclusions be?
> > Per
> > > > > file?
> > > > > > > Per
> > > > > > > > line?
> > > > > > > >
> > > > > > > > Best,
> > > > > > > > Piotrek
> > > > > > > >
> > > > > > > > wt., 25 kwi 2023 o 13:18 David Morávek <dm...@apache.org>
> > > > napisał(a):
> > > > > > > >
> > > > > > > >> Hi Everyone,
> > > > > > > >>
> > > > > > > >> A long time ago, the community decided not to use
> > Mockito-based
> > > > > tests
> > > > > > > >> because those are hard to maintain. This is already baked in
> > our
> > > > > Code
> > > > > > > Style
> > > > > > > >> and Quality Guide [1].
> > > > > > > >>
> > > > > > > >> Because we still have Mockito imported into the code base,
> > it's
> > > > very
> > > > > > > easy
> > > > > > > >> for newcomers to unconsciously introduce new tests violating
> > the
> > > > > code
> > > > > > > style
> > > > > > > >> because they're unaware of the decision.
> > > > > > > >>
> > > > > > > >> I propose to prevent Mockito usage with a Checkstyle rule
> for
> > a
> > > > new
> > > > > > > code,
> > > > > > > >> which would eventually allow us to eliminate it. This could
> > also
> > > > > > prevent
> > > > > > > >> some wasted work and unnecessary feedback cycles during
> > reviews.
> > > > > > > >>
> > > > > > > >> WDYT?
> > > > > > > >>
> > > > > > > >> [1]
> > > > > > > >>
> > > > > > > >>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#avoid-mockito---use-reusable-test-implementations
> > > > > > > >>
> > > > > > > >> Best,
> > > > > > > >> D.
> > > > > > > >>
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] Preventing Mockito usage for the new code with Checkstyle
Posted by Jing Ge <ji...@ververica.com.INVALID>.
Hi Panagiotis,
afaiu, those modules left are big and complex modules that still need a lot
of effort to migrate to junit5. Overall, agree with you to enable
suppressions.
Best regards,
Jing
On Mon, May 1, 2023 at 7:57 AM Panagiotis Garefalakis <pg...@apache.org>
wrote:
> Hey Jing,
>
> That's basically up to us to decide -- maybe a continuous approach is
> safer? (tests with junit4 behaviour won't be mergeable if we enable rules
> immediately)
> That said, looks like the migration work is almost there
> <https://issues.apache.org/jira/browse/FLINK-25325> -- with some effort we
> could minimize the need for suppressions altogether.
>
> Cheers,
> Panagiotis
>
> On Sun, Apr 30, 2023 at 6:16 AM Jing Ge <ji...@ververica.com.invalid>
> wrote:
>
> > Thanks @Panagiotis for the hint! Does it mean that those suppressions
> need
> > to be cleaned up continuously while we move forward with Junit5
> > migration(extra guideline is required), even if regex has been used? Or
> we
> > just leave them as they are and clean them up in one shot after every
> > junit4 has been migrated to junit5.
> >
> > Best regards,
> > Jing
> >
> > On Wed, Apr 26, 2023 at 9:02 AM Panagiotis Garefalakis <
> pgaref@apache.org>
> > wrote:
> >
> > > Thanks for bringing this up! +1 for the proposal
> > >
> > > @Jing Ge -- we don't necessarily need to completely migrate to Junit5
> > (even
> > > though it would be ideal).
> > > We could introduce the checkstyle rule and add suppressions for the
> > > existing problematic paths (as we do today for other rules e.g.,
> > > AvoidStarImport)
> > >
> > > Cheers,
> > > Panagiotis
> > >
> > > On Tue, Apr 25, 2023 at 11:48 PM Weihua Hu <hu...@gmail.com>
> > wrote:
> > >
> > > > Thanks for driving this.
> > > >
> > > > +1 for Mockito and Junit4.
> > > >
> > > > A clarity checkstyle will be of great help to new developers.
> > > >
> > > > Best,
> > > > Weihua
> > > >
> > > >
> > > > On Wed, Apr 26, 2023 at 1:47 PM Jing Ge <ji...@ververica.com.invalid>
> > > > wrote:
> > > >
> > > > > This is a great idea, thanks for bringing this up. +1
> > > > >
> > > > > Also +1 for Junit4. If I am not mistaken, it could only be done
> after
> > > the
> > > > > Junit5 migration is done.
> > > > >
> > > > > @Chesnay thanks for the hint. Do we have any doc about it? If not,
> it
> > > > might
> > > > > deserve one. WDYT?
> > > > >
> > > > > Best regards,
> > > > > Jing
> > > > >
> > > > > On Wed, Apr 26, 2023 at 5:13 AM Lijie Wang <
> wangdachui9501@gmail.com
> > >
> > > > > wrote:
> > > > >
> > > > > > Thanks for driving this. +1 for the proposal.
> > > > > >
> > > > > > Can we also prevent Junit4 usage in new code by this way？Because
> > > > > currently
> > > > > > we are aiming to migrate our codebase to JUnit 5.
> > > > > >
> > > > > > Best,
> > > > > > Lijie
> > > > > >
> > > > > > Piotr Nowojski <pn...@apache.org> 于2023年4月25日周二 23:02写道：
> > > > > >
> > > > > > > Ok, thanks for the clarification.
> > > > > > >
> > > > > > > Piotrek
> > > > > > >
> > > > > > > wt., 25 kwi 2023 o 16:38 Chesnay Schepler <ch...@apache.org>
> > > > > > napisał(a):
> > > > > > >
> > > > > > > > The checkstyle rule would just ban certain imports.
> > > > > > > > We'd add exclusions for all existing usages as we did when
> > > > > introducing
> > > > > > > > other rules.
> > > > > > > > So far we usually disabled checkstyle rules for a specific
> > files.
> > > > > > > >
> > > > > > > > On 25/04/2023 16:34, Piotr Nowojski wrote:
> > > > > > > > > +1 to the idea.
> > > > > > > > >
> > > > > > > > > How would this checkstyle rule work? Are you suggesting to
> > > start
> > > > > > with a
> > > > > > > > > number of exclusions? On what level will those exclusions
> be?
> > > Per
> > > > > > file?
> > > > > > > > Per
> > > > > > > > > line?
> > > > > > > > >
> > > > > > > > > Best,
> > > > > > > > > Piotrek
> > > > > > > > >
> > > > > > > > > wt., 25 kwi 2023 o 13:18 David Morávek <dm...@apache.org>
> > > > > napisał(a):
> > > > > > > > >
> > > > > > > > >> Hi Everyone,
> > > > > > > > >>
> > > > > > > > >> A long time ago, the community decided not to use
> > > Mockito-based
> > > > > > tests
> > > > > > > > >> because those are hard to maintain. This is already baked
> in
> > > our
> > > > > > Code
> > > > > > > > Style
> > > > > > > > >> and Quality Guide [1].
> > > > > > > > >>
> > > > > > > > >> Because we still have Mockito imported into the code base,
> > > it's
> > > > > very
> > > > > > > > easy
> > > > > > > > >> for newcomers to unconsciously introduce new tests
> violating
> > > the
> > > > > > code
> > > > > > > > style
> > > > > > > > >> because they're unaware of the decision.
> > > > > > > > >>
> > > > > > > > >> I propose to prevent Mockito usage with a Checkstyle rule
> > for
> > > a
> > > > > new
> > > > > > > > code,
> > > > > > > > >> which would eventually allow us to eliminate it. This
> could
> > > also
> > > > > > > prevent
> > > > > > > > >> some wasted work and unnecessary feedback cycles during
> > > reviews.
> > > > > > > > >>
> > > > > > > > >> WDYT?
> > > > > > > > >>
> > > > > > > > >> [1]
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#avoid-mockito---use-reusable-test-implementations
> > > > > > > > >>
> > > > > > > > >> Best,
> > > > > > > > >> D.
> > > > > > > > >>
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>