(re)activate rewrite capability

[Pankraz76]

(re)activate rewrite capability

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes

[marcphilipp]

assuming its not automated as automation of the automation of cosmetics was rejected assuming its not done for the real deal ether as build success but rewrite unable to be executed on my system:

I don't understand your sentence structure. Could you please elaborate?

PS: Wondering how you have it in place, even tho unused, asquarkus and maven (even escaled this) rejected it, because of recently changed licence foo.

We have it in place as a migration add from Java 8 to 17 for JUnit 6.0. I'm open to using it for static analysis and detecting issues if that's easy to set up.

[marcphilipp]

• enableMemoryMapping() is not supported on Java 24+ openrewrite/rewrite-static-analysis#613

Looks like this is indeed blocked until Open Rewrite updates to classgraph-4.8.180 or later.

[Pankraz76]

yes, but this seems to be an random impl. detail thats going to change again, like expected.

Could potentially fix by downgrade or just wait some time, as issue seems simple and closed soon.

This is more an strategic item wether or not to leverage to tool or kind of get rid or it, as the current benefit seems questionable.

[marcphilipp]

Looks like this is indeed blocked until Open Rewrite updates to classgraph-4.8.180 or later.

I double-checked and it's already using that version.

This is more an strategic item wether or not to leverage to tool or kind of get rid or it, as the current benefit seems questionable.

I'd like to give it a shot to see whether it's helpful. However, for that I'd have to try it out.

Blocked by openrewrite/rewrite#5677

[Pankraz76]

I don't understand your sentence structure. Could you please elaborate?

yes sorry.

As we dont use the auto apply on spot assuming its not done for rewrite ether.

Nice to see that you (at least) consider activating the failOnDryRunResults.

This shows real discipline and consequence.

[Pankraz76]

is it possible to have hybrid version of auto fix on dedicated local env value. As now it would be 2 goals to apply or extracting spot apply and rewrite apply into one profile, not to improve config burden on contributor, but giving conventional approach for this new convention that will be imposed.

this is silent approach making both parties happy.

• apply spotlessApply on dev environment #4705

[Pankraz76]

this could be considered mergeable. thanks for the interaction.

[Pankraz76]

this seems more consistent.

[Pankraz76]

Suggested change

activeRecipe("org.openrewrite.staticanalysis.EqualsAvoidsNull")

how do you prefer, might avoid this?

[vlsi]

@Pankraz76 , have you considered listing the "active recipes" in a yaml file so adding/removing would not trigger recompilation of the .build.gradle.kts files?

It could be like

# .build.gradle.kts
rewrite {
    configFile = file("config/rewrite.yml")
    activeRecipe("org.junit.openrewrite.recipe.CodeCleanup") // <-- refer a composite recipe from the config
}

# rewrite.yml

type: specs.openrewrite.org/v1beta/recipe
name: org.junit.openrewrite.recipe.CodeCleanup
displayName: Code cleanup
description: Automatically cleanup code, e.g. remove unnecessary parentheses, simplify expressions.
recipeList:
  # The list is taken from https://github.com/openrewrite/rewrite-static-analysis/blob/8c803a9c50b480841a4af031f60bac5ee443eb4e/src/main/resources/META-INF/rewrite/static-analysis.yml#L16C1-L42
  - org.openrewrite.staticanalysis.DefaultComesLast
  - org.openrewrite.staticanalysis.EmptyBlock
  - org.openrewrite.java.format.EmptyNewlineAtEndOfFile
  - org.openrewrite.staticanalysis.ForLoopControlVariablePostfixOperators
  - ...

[marcphilipp]

Now that OpenRewrite has released a version with JDK 24 support I was able to try out using rewriteDryRun and rewriteRun locally. I ran into an issue with OpenRewrite's Gradle plugin right away. Running Gradle with --no-parallel got me past that hurdle but that already rules out making rewriteDryRun part of check. We could have a separate CI stage for it but for that it would really have to provide value.

I enabled org.openrewrite.java.security.JavaSecurityBestPractices and org.openrewrite.staticanalysis.CommonStaticAnalysis. The first didn't report any findings, the second produced quite a lot of changes. Some of them were false positives or changes that broke compilation (e.g. UnnecessaryThrows, UseCollectionInterfaces, UnnecessaryExplicitTypeArguments) or unwanted (e.g. FinalClass, LambdaBlockToExpression, SimplifyBooleanReturn). Ultimately, the most useful/interesting and valid findings were produced by HideUtilityClassConstructor, EqualsAvoidsNull, and ModifierOrder.

All of these can also be reported by Checkstyle. While that doesn't provide support for automatic rewriting, there are usually only few issues reported when working on a PR. Therefore, instead of having to maintain configuration for OpenRewrite in addition to Checkstyle, I've enabled Checkstyle rules in #4748 and removed OpenRewrite from the build in cfd5845.

@Pankraz76 Thanks for the initiative! I did urge me to look into this more closely and clean up quite a few things, both in production code and build scripts.

[vlsi]

By the way, based on cfd5845, it looks like OpenRewrite is a no-go for JUnit unless openrewrite/rewrite-gradle-plugin#212 is resolved.

However, I find automatic replace String.format with "...".formatted by OpenRewrite much better than a pure complains by Checkstyle. The same goes for Order modifiers canonically, Remove empty statement, Hide utility class constructor and the other auto-fixable issues.

[marcphilipp]

However, I find automatic replace String.format with "...".formatted by OpenRewrite much better than a pure complains by Checkstyle.

Surprisingly to me, there were a lot of places OpenRewrite didn't find that a simple regex rule in Checkstyle did (see #4748).

[vlsi]

here were a lot of places OpenRewrite didn't find that a simple regex rule in Checkstyle did

Well, file an issue for OpenRewrite, wait a bit and later fix the missing entries :)
Having some amount of stale .format calls is bearable.

No way I push into OpenRewrite (it is clear the plugin is not usable for JUnit yet), however, I would like to stress that Checkstyle is hardly a replacement.

While that doesn't provide support for automatic rewriting, there are usually only few issues reported when working on a PR

Well, you are quite familiar with the code style you want to have, so you rarely run into the issues.
When it comes to external contributors, they are much more likely to hit the violations, and unfortunately, Checkstyle's errors are often cryptic for the end-user.

For instance, I try #4748 locally, and I add the following violation:

--- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TimeoutInvocationFactory.java
+++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TimeoutInvocationFactory.java
@@ -54,7 +54,7 @@ class TimeoutInvocationFactory {
        }

        @SuppressWarnings({ "deprecation", "try" })
-       private abstract static class ExecutorResource implements Store.CloseableResource, AutoCloseable {
+       private static abstract class ExecutorResource implements Store.CloseableResource, AutoCloseable {

                protected final ScheduledExecutorService executor;

The build failure is as follows:

> Task :junit-jupiter-engine:checkstyleMain
[ant:checkstyle] [ERROR] /.../junit-team/junit-framework/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TimeoutInvocationFactory.java:57:24:
'abstract' modifier out of order with the JLS suggestions. [ModifierOrder]

Well, sure everybody should know the JLS by heart (of course not), but I find such error messages quite unhelpful and demotivating.
The tool knows the modifier order it wants. It should rather print the desired state rather than complaining over a single modifier. ErrorProne is doing a much better job with error messages.

Unfortunately, Checkstyle maintainers do not even consider bad messages to be a Checkstyle problem. Checkstyle was never built with having good failure messages in the first place. At the same time, they virtually do not consider requests and pull requests from the users. I haven't seen that attitude changes over the past 11 years 😢

Sure your mileage might vary, however, the Java development would be amiable if Checkstyle usage could be reduced.

[marcphilipp]

@vlsi Thanks for your detailed comment! I'll keep that in mind. For now, the value OpenRewrite provided in this specific case didn't justify adding another tool to the build since we already have Checkstyle and Errorprone enabled.

[Pankraz76]

the reasonable decision to wait for the integrate of rewrite, until its fully fixed, does not apply to PMD, as it uses maven, right? @adangel

So we could leverage it there, for the very good explanation provided by @vlsi.

I haven't seen that attitude changes over the past 11 years 😢

Rewrite could change this.

Checkstyle usage could be reduced.

-> automated.

The idea of all these tools (rewrite, check, spot, PMD, and all them others), is one and the same.

Just how they reach it, is a different approach.

[Pankraz76]

but for that it would really have to provide value.

Assuming the fix is implemented soon, it appears Rewrite would have a significant impact by applying the 2-for-1 tradeoff for Spot and Check in favor of Rewrite.

This approach extends beyond simply including tools like PMD and FindBugs in scope-wise, but goes for full autofix mode, regarding these tools, covering alot of bug patterns, fixing what's adjustable.

It delivers the same charm and seamless integration as Spotless, but goes far beyond that.

[vlsi]

@Pankraz76, thanks for bringing up the topic!

Here’s how I imagine a practical path forward might look:

1. First, the issue in the OpenRewrite Gradle plugin needs to be resolved. That alone might be a non-trivial task.

2. Once that’s addressed, a deeper evaluation would be needed to ensure OpenRewrite integrates smoothly with JUnit’s tooling. For instance, JUnit occasionally uses release candidate Gradle versions—or even 9.0 immediately post-release—which could break compatibility. Gradle configuration cache and other advanced features may also pose challenges.

3. Finally, someone should assess which of the current Checkstyle/PMD/whatever rules OpenRewrite can reasonably replace. If it ends up covering only a small subset, it may be hard to justify introducing a new tool with minimal gain. From what I can tell, the JUnit maintainers are reasonably cautious about adding tools with “empty” or minimal default configurations.

One specific issue I’ve seen in the past: OpenRewrite didn’t handle Java-Kotlin mixed projects well because it struggled to resolve Kotlin types used from Java. That might not affect JUnit directly, but it’s worth noting.

From my perspective, @marcphilipp isn’t entirely opposed to OpenRewrite—it seems more about timing and current tradeoffs. At this point, the drawbacks appear to outweigh the benefits for JUnit.

Where things stand (as I see them):

• If the JUnit team decides that OpenRewrite just isn’t a fit—that’s totally fair, and the conversation would naturally end there. Regardless of enthusiasm, we have to respect that decision.

• If the team remains open to it, there’s probably no need for ongoing debate until the plugin is fixed. Once that’s done, it could be a good time to revisit the conversation with fresh data.

That’s what I was aiming for in my earlier comment: to lean things toward the “wait for fix, then reevaluate” position rather than closing the door entirely.

If there is a clear decision from the JUnit team (between those two options—or something else entirely!), I’d be grateful to hear it.

[marcphilipp]

I think the current concurrency issue is not the only concern with their Gradle plugin. Since the plugin doesn't support toolchains, it's likely that we'll run into another JDK compatibility issue in the future, should we decide to use it. Looks like the Moderne CLI could be a way to work around these limitations but that would mean extra work. For me, the bottom line is that the ends must justify the means and I'm not seeing that at the moment for this repository. If any of these constraints change in the future, I'm open to reconsider, though.

[Pankraz76]

Assuming it's only on large scale—which seems to be rare, mostly an initial occasion—once migrated, it should mostly function as a simple parser, diffing only a small delta due to tight iteration cycles.

Indeed, it runs stable as expected when shutting off Gradle's strongest performance benefit. Sequentially, the build takes 2–3x the time, which appears to be dedicated for cloud anyways, so it might be possible. Its just 2 mins.

Since regular PR´s have a very limited scope, operating in the dedicated (sub-)project is often sufficient, not require a full-scale run anyway.

Never the less, in my fork it seems done and remains concurrent, passing CI.

Assuming we mostly care for valid builds, a dryRun without any changes detected seems to make it, might be worth considering, as price of refactoring cheaper then cost of carry.

• [PoC] impose EqualsAvoidsNull Pankraz76/junit5#2

[Pankraz76]

It seems a good idea to maintain the effort spent on the Checkstyle configuration by simply automating it with Rewrite:

• Automatically Fix Checkstyle Violations with Rewrite

The integration ideally is seamless within the existing boundaries.
This could supplement or even replace Spot potentially, but Checkstyle should remain as a second layer of validation.