David and I had a discussion on GitHub, and I've been standardizing the changes along lines similar to what you described about ripping out anti-patterns. The reviews should be fairly straight forward because about 80% of the changes are just imports getting changed.
On Tue, Aug 31, 2021 at 6:06 PM Kevin Doran <[email protected]> wrote: > > 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 > >> >
