Hi all, JUnit 5 certainly contains a number of benefits, so I’m glad to see some interest and effort around it.
As long as we are taking the time to update tests in each module and line up reviewers for these bulk refactoring, I agree with David that we should make improvements aside from just JUnit 4 to 5 syntax/compatibility migrations. This is a good opportunity to bring consistency and improvements to our tests. Just an idea, but maybe it would be a good idea to start a list of anti-patterns and code smell in existing tests. This could be maintained on the wiki or similar, and any PRs opened as part of this effort can address those issues in addition to upgrading to JUnit 5. Here are examples of things I would include: Don’t: - Leave dead/commented code in test classes - @Ignored tests - Using try/catch with fail() to expect exceptions - Use thread.sleep() or other unreliable methods for asynchronous logic assertions Do: - Use granular unit tests - Use arrange/act/assert or given/when/then pattern where possible - Use assertThrows() to check for expected exceptions - Use libraries such as AssertJ or Awaitility to write clear, concise, reliable tests. - Use Spock where possible (Just joking! I know this is a heated issue. But also, do use it! 😂) Thanks! Kevin > On Aug 25, 2021, at 4:01 PM, David Handermann <[email protected]> > wrote: > > Mike, > > Thanks for moving things forward on the JUnit 5 migration. I posted a > couple comments on the PR for nifi-commons (NIFI-9080) with concerns > related to breaking tests and perpetuating ignored unit tests. > > Summarizing in this thread for general discussion, I am concerned about > breaking unit tests as part of the migration process. As long as NiFi > supports both JUnit 4 and JUnit 5, the migration should improve the overall > testing environment. Breaking existing tests will require additional work > to go back and fix, and spending the effort upgrading and reviewing test > classes doesn't seem worth it if we continue marking tests as ignored. In > some particular cases, it may be worthwhile to remove a test method or test > class. For test methods related to performance, migrating to JUnit 5 seems > like an ideal opportunity to make tests conditional on environment > variables or system properties. I would be glad to help with the migration > process, but it would be helpful to avoid having to revisit code multiple > times to address these issues. > > Regards, > David Handermann > > On Wed, Aug 25, 2021 at 2:24 PM Mike Thomsen <[email protected]> wrote: > >> I broke up the tickets because it is A LOT of individual tasks that >> can break the overall build and wanted to scope the work appropriately >> so that people looking for a chance to contribute could snatch up a >> few easy wins. >> >> I'm still going to take point on making the changes, but the plan >> going forward is to submit PRs that bundle a bunch of tickets so that >> we don't overwhelm reviewers and the CI/CD pipeline. >> >> Most of the changes are automated by IntelliJ Ultimate's migration >> tool, which from my testing is really good at migrating about 95% of >> our JUnit 4 unit and integration tests. >> >> Thanks, >> >> Mike >> >> On Wed, Aug 25, 2021 at 1:05 PM Joe Witt <[email protected]> wrote: >>> >>> Joey >>> >>> I personally dont care all that much about the number of commits in PR >>> - I think that rule is sort of soft already. >>> >>> I dont think there is any inherent value in having a single module per >>> JIRA (and PR or PR commit) on this. These can be done in much coarser >>> grained chunks. It will have to be to get review cycles for instance >>> (much less having the Github infra to run these builds). >>> >>> Thanks >>> Joe >>> >>> On Wed, Aug 25, 2021 at 9:44 AM Joey Frazee >>> <[email protected]> wrote: >>>> >>>> Maybe this is an exception to the single squashed commit guidance for >> the initial pull? >>>> >>>> I assume the intent is to make incremental progress and not have a PR >> with a hundred files affected, but if the different module changes >> corresponded to a different commit, GH will make it easy enough to have a >> draft and review each commit in isolation. >>>> >>>> Would that be a reasonable approach? >>>> >>>> -joey >>>> >>>>> On Aug 25, 2021, at 9:36 AM, Joe Witt <[email protected]> wrote: >>>>> >>>>> Mike, >>>>> >>>>> Seeing a pretty stunning flood of JIRAs for 'Refactor nifi-bla to use >>>>> JUnit 5' and I'm guessing we'll see the same in terms of PRs. This >> is >>>>> a really high administrative overhead approach to this. >>>>> >>>>> Why not break this into one or maybe a few JIRAs/PRs total? >>>>> >>>>> Thanks >>
