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 <ryan.skr...@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 <j...@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 <ruanhang1...@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 > > > > > >
