Mike, Thanks for raising this question. I have a couple thoughts, and I would be interested in perspectives from others.
Having worked on a number of performance and stability issues related to testing, there are varying degrees of test quality throughout the project. The collective result is long-running builds, particularly when it comes to GitHub workflows. Some of these issues require focused effort to correct, which makes it difficult to keep the scope of JUnit 5 migration focused. With that being said, limiting the scope to rearranging imports and expectations does not seem worth the effort. However, using the migration as an opportunity to perform basic cleanup would be very valuable. With that in mind, migration work seems to fit into three categories: 1. Simple replacement of imports: no structural changes needed 2. Refactoring deprecated approaches, such as replacing exception expectations with assertThrows, or replacing platform-specific assumptions with JUnit 5 annotations 3. Removing ignored tests or leveraging JUnit 5 annotations for System property-based enabling For modules that fit into the first category, a larger PR seems like the best way to go, since it should not require more than a cursory review. For modules in the second and third categories, smaller sets of modules are easier to review. With the module-based subtasks that you have already created, using a commit per subtask in a larger PR is a helpful approach. Given that some modules will require more significant refactoring, handling those changes in separate PRs will make it easier to keep the review process moving. Here is what I would like to see in the migration: 1. Elimination of ignored test methods: either complete method removal or changes to use EnabledIfSystemProperty. Using common property names like "nifi.test.performance" would be helpful 2. Elimination of commented-out or bypassed test methods 3. Replacement of platform-specific assumeTrue() references with DisabledOnOs annotations 4. Refactoring of other environment-specific assumptions using utility methods that could be leveraged using EnabledIf or DisabledIf annotations There are other items that would be helpful, but this seems like an opportunity to pay down some technical debt. With these goals in mind, the module scope for pull requests will vary on a case-by-case basis, but that seems like the best use of everyone's time. Thanks for the consideration. Regards, David Handermann On Fri, Aug 27, 2021 at 11:12 AM Mike Thomsen <[email protected]> wrote: > Some responders seem to prefer a batched migration while others have > suggested a massive PR that does all of the low-hanging fruit in one > push. I can do either, but would like to know what folks who can do > the reviews would prefer before going much deeper. > > Either way, most of the migration can be automated cleanly with > IntellJ Ultimate's migration tool. > > Thanks, > > Mike >
