Re: [DISCUSS] JUnit 5 Migration

[Mingliang Liu <li...@apache.org>]

>Some modules are already migrated by us, but the code hasn't been merged
since we still have some pending details to discuss.

I see you have created sub-task JIRAs for those modules in the umbrella
JIRA FLINK-25325 <https://issues.apache.org/jira/browse/FLINK-25325>. But
they are still unassigned so contributors may start working on those
modules only to find you have a pending code to merge. It's better to just
assign those sub-task JIRAs (to you or the author working on them) and mark
those JIRAs in-progress. What do you think?

Thanks,

On Wed, Jan 5, 2022 at 7:45 PM Hang Ruan <ru...@gmail.com> wrote:

> Hi, Ryan,
> Thanks a lot for helping with the migration. Some modules are already
> migrated by us, but the code hasn't been merged since we still have some
> pending details to discuss.
>
> These modules are flink-runtime,  flink-core, flink-test-utils,
> flink-runtime-web,
> flink-yarn, flink-kuberbetes, flink-dstl, flink-sql-parser,
> flink-clients, flink-streaming-java, flink-optimizer, flink-connector-base
> and flink-connector-kafka. We have created issues for these modules. Sorry
> for confusing.
>
> As for the commit author, we have some concerns about using individual
> authors for commits. Migrating to JUnit 5 will touch a huge number of code
> lines, and these changes are not quite helpful for tracing the actual
> evolution of these test cases using git blame. Last year the Flink project
> has a huge code reformatting commit touching almost all code (FLINK-20651
> <https://issues.apache.org/jira/browse/FLINK-20651>), and we ignore this
> commit in git blame to avoid polluting the git history. Maybe we can use
> the similar approach for JUnit migration commits.
>
> Best,
> Hang
>
> Ryan Skraba <ry...@aiven.io.invalid> 于2022年1月5日周三 18:39写道：
>
> > Hello!  I can help out with the effort -- I've got a bit of experience
> with
> > JUnit 4 and 5 migration, and it looks like even with the AssertJ scripts
> > there's going to be a lot of mechanical and manual work to be done.  The
> > migration document looks pretty comprehensive!
> >
> > For the remaining topics to be discussed:
> >
> > I don't have a strong opinion on what to do about parameterized tests
> that
> > use inheritance, although Jing Ge's proposal sounds reasonable and easy
> to
> > follow.  I wouldn't be worried about temporarily redundant test code too
> > much if it simplifies getting us into a good final state, especially
> since
> > redundant code would be easy to spot and remove when we get rid of JUnit
> 4
> > artifacts.
> >
> > Getting rid of PowerMock sounds fine to me.
> >
> > I don't think it's necessary to have a common author for commits, given
> > that commits will have the [JUnit5 migration] tag.  I guess my preference
> > would be to have "one or a few" commits per module, merged progressively.
> >
> > Is there an existing branch on a repo with some of the modules already
> > migrated?
> >
> > All my best, Ryan
> >
> > On Fri, Dec 17, 2021 at 5:19 PM Jing Ge <ji...@ververica.com> wrote:
> >
> > > Thanks Hang and Qingsheng for your effort and starting this discussion.
> > As
> > > additional information, I've created an umbrella ticket(
> > > https://issues.apache.org/jira/browse/FLINK-25325). It is recommended
> to
> > > create all JUnit5 migration related tasks under it, So we could track
> the
> > > whole migration easily.
> > >
> > > I think, for the parameterized test issue, the major problem is that,
> on
> > > one hand, JUnit 5 has its own approach to make parameterized tests and
> it
> > > does not allow to use parameterized fixtures at class level. This is a
> > huge
> > > difference compared to JUnit4. On the other hand, currently, there are
> > many
> > > cross module test class inheritances, which means that the migration
> > could
> > > not be done in one shot. It must be allowed to run JUnit4 and JUnit5
> > tests
> > > simultaneously during the migration process. As long as there are sub
> > > parameterized test classes in JUnit4, it will be risky to migrate the
> > > parent class to JUnit5. And if the parent class has to stick with
> JUnit4
> > > during the migration, any migrated JUnit5 subclass might need to
> > duplicate
> > > the test methods defined in the parent class. In this case, I would
> > prefer
> > > to duplicate the test methods with different names in the parent class
> > for
> > > both JUnit4 and JUnit5 only during the migration process as temporary
> > > solution and remove the test methods for JUnit4 once the migration
> > process
> > > is finished, i.e. when all subclasses are JUnit5 tests. It is a
> trade-off
> > > solution. Hopefully we could find another better solution during the
> > > discussion.
> > >
> > > Speaking of replacing @Test with @TestTemplate, since I did read all
> > tests,
> > > do we really need to replace all of them with @TestTemplate w.r.t. the
> > > parameterized tests?
> > >
> > > For the PowrMock tests, it is a good opportunity to remove them.
> > >
> > > best regards
> > > Jing
> > >
> > > On Fri, Dec 17, 2021 at 2:14 PM Hang Ruan <ru...@gmail.com>
> > wrote:
> > >
> > > > Hi, all,
> > > >
> > > > Apache Flink is using JUnit for unit tests and integration tests
> widely
> > > in
> > > > the project, however, it binds to the legacy JUnit 4 deeply. It is
> > > > important to migrate existing cases to JUnit 5 in order to avoid
> > > splitting
> > > > the project into different JUnit versions.
> > > >
> > > > Qingsheng Ren and I have conducted some trials about the JUnit 5
> > > migration,
> > > > but there are too many modules that need to migrate. We would like to
> > get
> > > > more help from the community. It is planned to migrate module by
> > module,
> > > > and a JUnit 5 migration guide
> > > > <
> > > >
> > >
> >
> https://docs.google.com/document/d/1514Wa_aNB9bJUen4xm5uiuXOooOJTtXqS_Jqk9KJitU/edit?usp=sharing
> > > > >[1]
> > > > has been provided to new helpers on the cooperation method and how to
> > > > migrate.
> > > >
> > > > There are still some problem to discuss:
> > > >
> > > > 1. About parameterized test:
> > > >
> > > > Some test classes inherit from other base test classes. We have
> > discussed
> > > > different situations in the guide, but the situation where a
> > > parameterized
> > > > test subclass inherits from a non parameterized parent class has not
> > been
> > > > resolved.
> > > >
> > > > In JUnit 4, the parent test class always has some test cases
> annotated
> > by
> > > > @Test. And  the parameterized subclass will run these test cases in
> the
> > > > parent class in a parameterized way.
> > > >
> > > > In JUnit 5, if we want a test case to be invoked multiple times, the
> > test
> > > > case must be annotated by @TestTemplate. A test case can not be
> > annotated
> > > > by both @Test and @TestTemplate, which means a test case can not be
> > > invoked
> > > > as both a parameterized test and a non parameterized test.
> > > >
> > > > We thought of two ways to migrate this situation, but not good
> enough.
> > > Both
> > > > two ways will introduce redundant codes, and make it hard to
> maintain.
> > > >
> > > > The first way is to change the parent class to a parameterized test
> and
> > > > replace @Test tests to @TestTemplate tests. For its non parameterized
> > > > subclasses, we provide them a fake parameter method, which will
> provide
> > > > nothing.
> > > >
> > > > The second way is to change the parameterized subclasses. We should
> > > > override all @Test tests in the parent class and annotate them with
> > > > @TestTemplate, these methods in the parameterized subclass should
> > invoke
> > > > the same method in its parent class.
> > > >
> > > >
> > > > 2. About PowerMock:
> > > >
> > > > According to the code style rules[2] of Flink project, it’s
> discouraged
> > > to
> > > > use mockito for test cases, as well as Powermock, an extension of
> > > mockito.
> > > > Considering the situation that PowerMock lacks JUnit 5 support [3],
> we
> > > > suggest rewriting Powermock-based test class (~10 classes) with
> > reusable
> > > > test implementations, and finally deprecate Powermock from the
> project.
> > > >
> > > >
> > > > 3. Commit Author:
> > > >
> > > > JUnit 5 migration will cause a lot of changed codes. Maybe we should
> > use
> > > a
> > > > common author for the JUnit 5 migration commits.
> > > >
> > > > Looking forward to your suggestions, thanks!
> > > >
> > > > Best,
> > > >
> > > > Hang
> > > >
> > > > [1]
> > > >
> > > >
> > >
> >
> https://docs.google.com/document/d/1514Wa_aNB9bJUen4xm5uiuXOooOJTtXqS_Jqk9KJitU/edit?usp=sharing
> > > >
> > > > [2]
> > > >
> > > >
> > >
> >
> https://flink.apache.org/contributing/code-style-and-quality-common.html#avoid-mockito---use-reusable-test-implementations
> > > >
> > > >
> > > > [3] https://github.com/powermock/powermock/issues/929
> > > >
> > >
> >
>