Thanks Arvid for the feedback! I think merging the giant commit after cutting 1.14 release branch would be a good idea.
Since no one has objections in the discussion, I’d like to move a step forward and raise a vote on the migration. Looking forward to having JUnit 5 in the 1.14 release cycle! -- Best Regards, Qingsheng Ren Email: renqs...@gmail.com On Jun 29, 2021, 9:20 PM +0800, Arvid Heise <ar...@apache.org>, wrote: > Hi Qingsheng, > > I like the idea of enforcing JUnit5 tests with checkstyle. I'm assuming > JUnit4 will bleed from time to time into the test classpath. > > Obviously, we can only do that after all tests are migrated and we are > confident that no small change would require a contributor to do the > migration of a test in an unrelated change. > > For the big commit, I'd propose to have it after branch cut for 1.14 > release. So for 1.14 we would just have the coexistance PR with the vintage > engine. In that way, the least possible number of contributors should be > affected. Of course, the big commit can be prepared beforehand. > > On Tue, Jun 29, 2021 at 11:44 AM Qingsheng Ren <renqs...@gmail.com> wrote: > > > Thanks for wrapping things up and effort on the migration Arvid! > > > > I’m +1 for the migration plan. > > > > To summarize the migration path proposed by Arvid: > > > > 1. Remove JUnit 4 dependency and introduce junit5-vintage-engine in the > > project (all existing cases will still work) > > 2. Rewrite JUnit 4 rules in JUnit 5 extension style (~10 rules) > > 3. Migrate all existing tests to JUnit 5 (This is a giant commit similar > > to code formatting) > > 4. Remove vintage runner and some cleanup > > > > One issue is that we cannot totally get rid of JUnit 4 dependency from the > > project because: > > > > 1. Testcontainers. As mentioned in their official docs[1], Testcontainers > > still depends on JUnit 4, and the problem might not be solved until > > Testcontainer 2, which still has no roadmap[2]. > > 2. It’s still possible to appear as transitive dependency > > > > Since classes of JUnit 4 and 5 are under different packages, there won’t > > be conflict having JUnit 4 in the project. To prevent the project splitting > > into 4 & 5 again, we can ban JUnit 4 imports in CheckStyle to prevent > > developers to write test cases in JUnit 4 style intentionally or mistakenly. > > > > I’m happy and willing to take over the migration work. This migration > > indeed takes some efforts, but it will help with test case developing in > > the future. > > > > [1] https://www.testcontainers.org/test_framework_integration/junit_5/ > > [2] > > https://github.com/testcontainers/testcontainers-java/issues/970#issuecomment-437273363 > > > > -- > > Best Regards, > > > > Qingsheng Ren > > Email: renqs...@gmail.com > > On Jun 16, 2021, 3:13 AM +0800, Arvid Heise <ar...@ververica.com>, wrote: > > > Sorry for following up so late. A while ago, I spiked a junit 5 > > migration. > > > > > > To recap: here is the migration plan. > > > > > > 0. (There is a way to use JUnit4 + 5 at the same time in a project - > > you'd > > > > use a specific JUnit4 runner to execute JUnit5. I'd like to skip this > > > > step as it would slow down migration significantly) > > > > 1. Use JUnit5 with vintage runner. JUnit4 tests run mostly out of the > > > > box. The most important difference is that only 3 base rules are > > supported > > > > and the remainder needs to be migrated. Luckily, most of our rules > > derive > > > > from the supported ExternalResource. So in this step, we would need to > > > > migrate the rules. > > > > 2. Implement new tests in JUnit5. > > > > 3. Soft-migrate old tests in JUnit5. This is mostly a renaming of > > > > annotation (@Before -> @BeforeEach, etc.). Adjust parameterized tests > > > > (~400), replace rule usages (~670) with extensions, exception handling > > > > (~1600 tests), and timeouts (~200). This can be done on a test class by > > > > test class base and there is no hurry. > > > > 4. Remove vintage runner, once most tests are migrated by doing a final > > > > push for lesser used modules. > > > > > > > > > > Here are my insights: > > > 0. works but I don't see the benefit > > > 1. works well [1] with a small diff [2]. Note that the branch is based > > on a > > > quite old master. > > > 2. works well as well [3]. > > > 2a. However, we should be aware that we need to port quite a few rules to > > > extensions before we can implement more complex JUnit5 tests, especially > > > ITCases (I'd probably skip junit-jupiter-migrationsupport that allows us > > to > > > reuse _some_ rules using specific base classes). We have ~10-15 rules > > that > > > need to be ported. > > > 3. Soft migration will take forever and probably never finish. Many tests > > > can be automatically ported with some (I used 8) simple regexes. I'd > > rather > > > do a hard migration of all tests at a particular point (no freeze) and > > have > > > that git commit excluded from blame, similar to the spotless commit. > > > 3a. A huge chunk of changes (>90%) comes from the optional message in > > > assertX being moved from the first to the last position. @Chesnay > > Schepler > > > <ches...@apache.org> proposed to rather implement our own Assertion > > class > > > in the old junit package that translates it. But this would need to go > > hand > > > in hand with 4) to avoid name clash. It could also just be a temporary > > > thing that we use during hard migration and then inline before merging. > > > 4. If we do hard migration, we should probably do that in a follow-up PR > > > (contains just the migrations of the tests that have been added in the > > > meantime). > > > > > > Here is my time-limited take on the hard migration [4]. It was a matter > > of > > > ~10h. > > > > > > [1] > > > > > https://dev.azure.com/arvidheise0209/arvidheise/_build/results?buildId=1208&view=ms.vss-test-web.build-test-results-tab&runId=24838&resultId=100000&paneView=debug > > > [2] > > > > > https://github.com/AHeise/flink/commit/7f3e7faac9ba53615bda89e51d5fd17d940c4a55 > > > [3] > > > > > https://github.com/AHeise/flink/commit/c0dd3d12fbd07b327b560107396ee0bb1e2d8969 > > > [4] > > https://github.com/apache/flink/compare/master...AHeise:junit5?expand=1 > > > > > > On Tue, Dec 1, 2020 at 9:54 AM Khachatryan Roman < > > > khachatryan.ro...@gmail.com> wrote: > > > > > > > +1 for the migration > > > > > > > > (I agree with Dawid, for me the most important benefit is better > > support of > > > > parameterized tests). > > > > > > > > Regards, > > > > Roman > > > > > > > > > > > > On Mon, Nov 30, 2020 at 9:42 PM Arvid Heise <ar...@ververica.com> > > wrote: > > > > > > > > > Hi Till, > > > > > > > > > > immediate benefit would be mostly nested tests for a better test > > > > structure > > > > > and new parameterized tests for less clutter (often test > > functionality is > > > > > split into parameterized test and non-parameterized test because of > > > > JUnit4 > > > > > limitation). Additionally, having Java8 lambdas to perform fine-grain > > > > > exception handling would make all related tests more readable (@Test > > only > > > > > allows one exception per test method, while in reality we often have > > more > > > > > exceptions / more fine grain assertions and need to resort to > > try-catch > > > > -- > > > > > yuck!). The extension mechanism would also make the mini cluster much > > > > > easier to use: we often have to start the cluster manually because of > > > > > test-specific configuration, which can be easily avoided in JUnit5. > > > > > > > > > > In the medium and long-term, I'd also like to use the modular > > > > > infrastructure and improved parallelization. The former would allow > > us > > > > > better to implement cross-cutting features like TestLogger (why do we > > > > need > > > > > to extend that manually in every test?). The latter is more relevant > > for > > > > > the next push on CI, which would be especially interesting with e2e > > being > > > > > available in Java. > > > > > > > > > > On Mon, Nov 30, 2020 at 2:07 PM Dawid Wysakowicz < > > dwysakow...@apache.org > > > > > > > > > > wrote: > > > > > > > > > > > Hi all, > > > > > > > > > > > > Just wanted to express my support for the idea. I did miss certain > > > > > > features of JUnit 5 already, an important one being much better > > support > > > > > > for parameterized tests. > > > > > > > > > > > > Best, > > > > > > > > > > > > Dawid > > > > > > > > > > > > On 30/11/2020 13:50, Arvid Heise wrote: > > > > > > > Hi Chesnay, > > > > > > > > > > > > > > The vintage runner supports the old annotations, so we don't > > have to > > > > > > change > > > > > > > them in the first step. > > > > > > > > > > > > > > The only thing that we need to change are all rules that do not > > > > extend > > > > > > > ExternalResource (e.g., TestWatcher used in TestLogger). This > > change > > > > > > needs > > > > > > > to be done swiftly as this affects the shared infrastructure as > > you > > > > > have > > > > > > > mentioned. > > > > > > > > > > > > > > Only afterwards, we start to actually migrate the individual > > tests. > > > > > That > > > > > > > can be done module by module or as we go. I actually found a nice > > > > > article > > > > > > > that leverages the migration assist of IntelliJ [1]. > > > > > > > > > > > > > > As the last stop, we remove the vintage runner - all JUnit4 tests > > > > have > > > > > > been > > > > > > > migrated and new tests cannot use old annotation etc. anymore. > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > https://blog.jetbrains.com/idea/2020/08/migrating-from-junit-4-to-junit-5/ > > > > > > > > > > > > > > On Mon, Nov 30, 2020 at 1:32 PM Chesnay Schepler < > > ches...@apache.org > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > I presume we cannot do the migration module-wise due to shared > > test > > > > > > > > utilities that rely on JUnit interfaces? > > > > > > > > > > > > > > > > On 11/30/2020 1:30 PM, Chesnay Schepler wrote: > > > > > > > > > Is it feasible that 2 people can do the migration within a > > short > > > > > > > > > time-frame (say, a week)? > > > > > > > > > Must the migration of a test be done in one go, or can we for > > > > example > > > > > > > > > first rename all the Before/After annotations and then to > > the rest? > > > > > > > > > Are there any issues with other test dependencies (i.e., > > hamcrest, > > > > > > > > > powermock (PowerMockRunner), mockito) that we should be > > aware of? > > > > > > > > > > > > > > > > > > I generally like the idea of using JUnit 5, but am wary of > > this > > > > > > > > > migration dragging on for too long. > > > > > > > > > > > > > > > > > > On 11/27/2020 3:29 PM, Arvid Heise wrote: > > > > > > > > > > Dear devs, > > > > > > > > > > > > > > > > > > > > I'd like to start a discussion to migrate to a higher JUnit > > > > version. > > > > > > > > > > > > > > > > > > > > The main motivations are: > > > > > > > > > > - Making full use of Java 8 Lambdas for writing easier to > > read > > > > tests > > > > > > > > > > and a > > > > > > > > > > better performing way of composing failure messages. > > > > > > > > > > - Improved test structures with nested and dynamic tests. > > > > > > > > > > - Much better support for parameterized tests to avoid > > separating > > > > > > > > > > parameterized and non-parameterized parts into different > > test > > > > > classes. > > > > > > > > > > - Composable dependencies and better hooks for advanced > > use cases > > > > > > > > > > (TestLogger). > > > > > > > > > > - Better exception verification > > > > > > > > > > - More current infrastructure > > > > > > > > > > - Better parallelizable > > > > > > > > > > > > > > > > > > > > Why now? > > > > > > > > > > - JUnit5 is now mature enough to consider it for such a > > complex > > > > > > project > > > > > > > > > > - We are porting more and more e2e tests to JUnit and it > > would be > > > > a > > > > > > > > > > pity to > > > > > > > > > > do all the work twice (okay some already has been done and > > would > > > > > > > > > > result in > > > > > > > > > > adjustments, but the sooner we migrate, the less needs to > > be > > > > touched > > > > > > > > > > twice) > > > > > > > > > > > > > > > > > > > > Why JUnit5? > > > > > > > > > > There are other interesting alternatives, such as TestNG. > > I'm > > > > happy > > > > > > > > > > to hear > > > > > > > > > > specific alternatives. For now, I'd like to focus on > > JUnit4 for an > > > > > > > > > > easier > > > > > > > > > > migration path. > > > > > > > > > > > > > > > > > > > > Please discuss if you would also be interested in moving > > onward. > > > > To > > > > > > get > > > > > > > > > > some overview, I'd like to see some informal +1 for the > > options: > > > > > > > > > > > > > > > > > > > > [ ] Stick to JUnit4 for the time being > > > > > > > > > > [ ] Move to JUnit5 (see migration path below) > > > > > > > > > > [ ] Alternative idea + advantages over JUnit5 + some very > > rough > > > > > > > > > > migration > > > > > > > > > > path > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > Migrating from JUnit4 to JUnit5 can be done in some steps, > > so that > > > > > we > > > > > > > > > > can > > > > > > > > > > gradually move from JUnit4 to JUnit5. > > > > > > > > > > > > > > > > > > > > 0. (There is a way to use JUnit4 + 5 at the same time in a > > > > project - > > > > > > > > > > you'd > > > > > > > > > > use a specific JUnit4 runner to execute JUnit5. I'd like > > to skip > > > > > this > > > > > > > > > > step > > > > > > > > > > as it would slow down migration significantly) > > > > > > > > > > 1. Use JUnit5 with vintage runner. JUnit4 tests run mostly > > out of > > > > > the > > > > > > > > > > box. > > > > > > > > > > The most important difference is that only 3 base rules are > > > > > supported > > > > > > > > > > and > > > > > > > > > > the remainder needs to be migrated. Luckily, most of our > > rules > > > > > derive > > > > > > > > > > from > > > > > > > > > > the supported ExternalResource. So in this step, we would > > need to > > > > > > > > > > migrate > > > > > > > > > > the rules. > > > > > > > > > > 2. Implement new tests in JUnit5. > > > > > > > > > > 3. Soft-migrate old tests in JUnit5. This is mostly a > > renaming of > > > > > > > > > > annotation (@Before -> @BeforeEach, etc.). Adjust > > parameterized > > > > > tests > > > > > > > > > > (~400), replace rule usages (~670) with extensions, > > exception > > > > > handling > > > > > > > > > > (~1600 tests), and timeouts (~200). This can be done on a > > test > > > > class > > > > > > by > > > > > > > > > > test class base and there is no hurry. > > > > > > > > > > 4. Remove vintage runner, once most tests are migrated by > > doing a > > > > > > final > > > > > > > > > > push for lesser used modules. > > > > > > > > > > > > > > > > > > > > Let me know what you think and I'm happy to answer all > > questions. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > Arvid Heise | Senior Java Developer > > > > > > > > > > <https://www.ververica.com/> > > > > > > > > > > Follow us @VervericaData > > > > > > > > > > -- > > > > > > > > > > Join Flink Forward <https://flink-forward.org/> - The Apache Flink > > > > > Conference > > > > > > > > > > Stream Processing | Event Driven | Real Time > > > > > > > > > > -- > > > > > > > > > > Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany > > > > > > > > > > -- > > > > > Ververica GmbH > > > > > Registered at Amtsgericht Charlottenburg: HRB 158244 B > > > > > Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, > > Ji > > > > > (Toni) Cheng > > > > > > > > > > > > > > > > > > -- > > > > > > Arvid Heise | Senior Java Developer > > > > > > <https://www.ververica.com/> > > > > > > Follow us @VervericaData > > > > > > -- > > > > > > Join Flink Forward <https://flink-forward.org/> - The Apache Flink > > > Conference > > > > > > Stream Processing | Event Driven | Real Time > > > > > > -- > > > > > > Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany > > > > > > -- > > > Ververica GmbH > > > Registered at Amtsgericht Charlottenburg: HRB 158244 B > > > Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji > > > (Toni) Cheng > >
