>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 <ruanhang1...@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 <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 > > > > > > > > > >
