You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flink.apache.org by Hang Ruan <ru...@gmail.com> on 2021/12/17 13:14:12 UTC
[DISCUSS] JUnit 5 Migration
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
Re: [DISCUSS] JUnit 5 Migration
Posted by 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
> > > >
> > >
> >
>
Re: [DISCUSS] JUnit 5 Migration
Posted by Hang Ruan <ru...@gmail.com>.
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
> > >
> >
>
Re: [DISCUSS] JUnit 5 Migration
Posted by Ryan Skraba <ry...@aiven.io.INVALID>.
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
> >
>
Re: [DISCUSS] JUnit 5 Migration
Posted by Jing Ge <ji...@ververica.com>.
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
>