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 >
