build: adopt OpenRewrite with a hand-curated recipe list - #3308
Merged
Conversation
Wire the OpenRewrite Gradle plugin at the root project and pin the recipe list in rewrite.yml. No source changes. The upstream composites are deliberately not activated. OpenRewrite offers no way to exclude a single sub-recipe from a composite -- exclusion() filters file paths only, and the upstream request is closed as won't-fix (openrewrite/rewrite#1714). Measured on src/main, CommonStaticAnalysis touches 101 files; about half overlaps with the list we want, and the rest comes from recipes that must not run here: ExplicitInitialization (46), FinalClass (12), FinalizePrivateFields (10), AbstractClassPublicConstructor (6) and ReplaceLambdaWithMethodReference (4) alter an OSGi-exported API with no binary-compatibility gate, or add final to fields TestNG writes reflectively. Java8toJava11 is rejected separately: it would add JAXB, JAX-WS, javax.inject and javax.annotation runtime dependencies to a library that deliberately has almost none. Two things make the setup behave: An ImportLayoutStyle matching google-java-format is declared, because OpenRewrite otherwise auto-detects a layout that groups java.*/javax.* separately. Measured, OrderImports rewrites 34 files under auto-detection and 1 with the style active. failOnDryRunResults is wired to a new failOnRewriteDryRun build parameter rather than hard-coded, so the CI gate is the Gradle task itself and ./gradlew rewriteDryRun -PfailOnRewriteDryRun=true reproduces it locally. This also requires build-logic.build-params at the root, which was not applied there. Scope is src/main only. Test sources are excluded because test/** mixes real tests with fixture classes whose method names, declaration order and finalize() presence are what the surrounding tests assert on, and because testng.xml references test classes by fully qualified name, so a moved class would silently drop tests.
skipAutostyle has been declared in build-parameters since it was introduced, but nothing ever read it: testng.java applied testng.style unconditionally from its plugins block. Passing -PskipAutostyle=true therefore did nothing at all, while the neighbouring -PskipErrorProne=true worked, because that one is applied conditionally at the bottom of the same file. Apply testng.style the same way Error Prone is applied, so the two parameters behave consistently. Verified by appending a deliberate formatting violation to a source file: ./gradlew :testng-collections:check -PskipAutostyle=true -> passes (was: failed) ./gradlew :testng-collections:check -> fails on the violation Note this means the autostyle tasks do not exist when the parameter is set, so invoking autostyleCheck explicitly alongside it fails with "task not found". That matches how skipErrorProne already behaves; the parameter is meant for whole-build invocations such as ./gradlew build -PskipAutostyle=true.
Without a gate the cleanup decays: the next pull request reintroduces the idioms
the recipes just removed, and nothing notices.
The gate is the Gradle task, not shell glue -- the job passes
failOnRewriteDryRun=true, so ./gradlew rewriteDryRun -PfailOnRewriteDryRun=true
reproduces it exactly. The shell step only summarises the patch on failure, and
the full patch is uploaded as an artifact.
Design notes:
* It is its own job, not part of the build matrix, which produces 8 jobs across
OS/JDK/timezone/locale. It has no "needs", so it starts immediately and runs
alongside the matrix; wall-clock impact is nil.
* rewriteDryRun stays out of check/build, so a normal local build is unaffected.
* compileTestJava/compileTestKotlin are excluded. rewriteDryRun dependsOn every
source set's compileJava, but test sources are excluded from rewriting, so
compiling them feeds nothing into the LST. Error Prone is skipped for the same
reason -- it is a javac plugin OpenRewrite never sees, and the matrix runs it.
Measured roughly 86s -> 41s locally, with the gate still failing on drift.
* -Xmx3g is provisioned: rewriteDryRun builds a type-attributed LST of every
main source file and OOMed once under Gradle's default heap.
One caveat worth stating: this trains contributors to run rewriteRun to make CI
green. That is only acceptable because the recipe list is curated and each entry
was reviewed -- a hard gate on a composite recipe would be teaching people to
apply unreviewed semantic rewrites.
Apply org.testng.build.ModernizeMainSources, then ./gradlew autostyleApply so google-java-format has the last word on formatting. 45 files, all under src/main. Recipes that matched: NeedBraces 22 UnnecessaryParentheses 22 InlineVariable 11 EqualsAvoidsNull 3 RemoveExtraSemicolons 2 MultipleVariableDeclarations 1 ReplaceStringConcatenationWithStringValueOf 1 org.openrewrite.java.migrate.nio.file.RedundantUtf8Charset 1 The other 44 recipes in the list matched nothing. That is the honest yield: the Java 8 to Java 11 API migration is worth exactly one file, because this codebase was already clean of the idioms those recipes target. Notes on the hunks that are not purely syntactic: EqualsAvoidsNull flips "value.equals(LITERAL)" to "LITERAL.equals(value)". Where value is null this yields false instead of throwing NullPointerException; every call site here is a filter predicate or a branch condition where false is the correct answer, and in one case it subsumes an explicit null check. UnnecessaryParentheses only drops parentheses redundant within their own expression, keeping the grouping pair, so "a + ((x == null) ? 0 : y)" becomes "a + (x == null ? 0 : y)" rather than losing precedence. RedundantUtf8Charset drops the explicit UTF_8 argument from Files.newBufferedWriter, which documents UTF-8 as its default. No test source and no build script is touched, so the set of executed tests is structurally unchanged. Full build green: 12624 tests, 0 failures.
Describe rewriteDryRun/rewriteRun in CONTRIBUTING.md, stress that autostyleApply must run afterwards as a separate Gradle invocation, give the exact command that reproduces the CI gate, point at rewrite.yml as the recipe list, and note the configuration-cache incompatibility. Add a CHANGES.txt entry.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughOpenRewrite is configured with selected recipes and import formatting, integrated into Gradle and CI, documented for contributors, and applied to Java main-source files through syntax, null-safety, control-flow, and formatting changes. ChangesOpenRewrite modernization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RewriteCI
participant GradleRewrite
participant RewriteRecipes
participant RewritePatch
RewriteCI->>GradleRewrite: run rewriteDryRun with failOnRewriteDryRun=true
GradleRewrite->>RewriteRecipes: load ModernizeMainSources and ImportLayout
RewriteRecipes->>RewritePatch: generate rewrite.patch for remaining changes
RewriteCI->>RewritePatch: upload patch artifact when the job fails
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/CONTRIBUTING.md:
- Around line 117-120: Label the fenced shell command block in the contributing
documentation by adding the shell language identifier to its opening fence,
while leaving the commands unchanged.
In @.github/workflows/test.yml:
- Around line 40-42: Add a job-level permissions block to the rewrite job,
identified by the rewrite key, granting only contents: read. Keep the existing
job name and runner configuration unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2acd06bb-c6e7-4711-95d3-eaca9e6f59b4
📒 Files selected for processing (52)
.github/CONTRIBUTING.md.github/workflows/test.ymlCHANGES.txtbuild-logic/build-parameters/build.gradle.ktsbuild-logic/jvm/src/main/kotlin/testng.java.gradle.ktsbuild.gradle.ktsrewrite.ymltestng-collections/src/main/java/org/testng/collections/Objects.javatestng-core-api/src/main/java/org/testng/internal/AutoCloseableLock.javatestng-core-api/src/main/java/org/testng/internal/ConstructorOrMethod.javatestng-core-api/src/main/java/org/testng/internal/KeyAwareAutoCloseableLock.javatestng-core-api/src/main/java/org/testng/internal/Utils.javatestng-core-api/src/main/java/org/testng/reporters/FileStringBuffer.javatestng-core-api/src/main/java/org/testng/reporters/XMLUtils.javatestng-core-api/src/main/java/org/testng/xml/XmlClass.javatestng-core-api/src/main/java/org/testng/xml/XmlDefine.javatestng-core-api/src/main/java/org/testng/xml/XmlInclude.javatestng-core-api/src/main/java/org/testng/xml/XmlMethodSelector.javatestng-core-api/src/main/java/org/testng/xml/XmlPackage.javatestng-core-api/src/main/java/org/testng/xml/XmlSuite.javatestng-core-api/src/main/java/org/testng/xml/XmlTest.javatestng-core/src/main/java/org/testng/Converter.javatestng-core/src/main/java/org/testng/JarFileUtils.javatestng-core/src/main/java/org/testng/SuiteRunner.javatestng-core/src/main/java/org/testng/TestRunner.javatestng-core/src/main/java/org/testng/internal/ClassBasedWrapper.javatestng-core/src/main/java/org/testng/internal/IObject.javatestng-core/src/main/java/org/testng/internal/MethodHelper.javatestng-core/src/main/java/org/testng/internal/MethodInstance.javatestng-core/src/main/java/org/testng/internal/Parameters.javatestng-core/src/main/java/org/testng/internal/TestNGClassFinder.javatestng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.javatestng-core/src/main/java/org/testng/internal/XmlMethodSelector.javatestng-core/src/main/java/org/testng/internal/Yaml.javatestng-core/src/main/java/org/testng/internal/annotations/AnnotationHelper.javatestng-core/src/main/java/org/testng/internal/collections/Pair.javatestng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.javatestng-core/src/main/java/org/testng/internal/invokers/MethodRunner.javatestng-core/src/main/java/org/testng/internal/invokers/ParameterHandler.javatestng-core/src/main/java/org/testng/internal/objects/GuiceHelper.javatestng-core/src/main/java/org/testng/internal/objects/ObjectFactoryImpl.javatestng-core/src/main/java/org/testng/internal/reflect/ReflectionRecipes.javatestng-core/src/main/java/org/testng/reporters/EmailableReporter2.javatestng-core/src/main/java/org/testng/reporters/JUnitReportReporter.javatestng-core/src/main/java/org/testng/reporters/MethodInvocationKey.javatestng-core/src/main/java/org/testng/reporters/SuiteHTMLReporter.javatestng-core/src/main/java/org/testng/reporters/VerboseReporter.javatestng-core/src/main/java/org/testng/reporters/jq/Model.javatestng-core/src/main/java/org/testng/reporters/jq/NavigatorPanel.javatestng-core/src/main/java/org/testng/reporters/jq/TestNgXmlPanel.javatestng-core/src/main/java/org/testng/xml/TestNGContentHandler.javatestng-runner-api/src/main/java/org/testng/internal/LiteWeightTestNGMethod.java
The job only checks out the repository and uploads an artifact, so grant it contents: read explicitly rather than letting it inherit the workflow default. Other workflows here already declare permissions (combine-prs, label-commenter); test.yml did not. Also label the shell block added to CONTRIBUTING.md, matching the bash fence the file already uses for its other command block.
Member
Author
|
@krmahadevan LGTM, ready for your review |
krmahadevan
approved these changes
Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #3292, where @vlsi mentioned planning to move the Java part of the formatting setup to
OpenRewrite. This PR does not touch Autostyle — it adds OpenRewrite alongside it, for
refactoring rather than formatting, and makes the two agree on import layout.
What
Adds OpenRewrite to the build and applies a hand-picked set of recipes to the main sources only.
Five commits, reviewable independently:
build:add OpenRewrite with a hand-curated recipe listbuild:make theskipAutostylebuild parameter actually workci:fail the build when OpenRewrite still has changes to makeOpenRewritejobrefactor:apply the OpenRewrite recipes to the main sourcessrc/maindocs:document the OpenRewrite workflowRecipes applied
The full list lives in
rewrite.yml. What actually matched:NeedBracesUnnecessaryParenthesesInlineVariableEqualsAvoidsNullRemoveExtraSemicolonsMultipleVariableDeclarationsReplaceStringConcatenationWithStringValueOforg.openrewrite.java.migrate.nio.file.RedundantUtf8CharsetThe other 44 recipes in the list matched nothing and are kept so future contributions are covered
by the same list.
Honest note on the Java 8 → Java 11 migration: it is worth one file. Every other leaf of
Java8toJava11matched nothing, because this codebase was already clean of the idioms it targets.Why the upstream composites are not used
CommonStaticAnalysis,CodeCleanupandJava8toJava11are not activated. OpenRewrite has noway to exclude a single sub-recipe from a composite —
exclusion()filters file paths only, andthe upstream request is closed as won't-fix
(openrewrite/rewrite#1714).
Measured on
src/main,CommonStaticAnalysistouches 101 files. About half overlaps with thelist we want; the rest comes from recipes that must not run here:
ExplicitInitializationFinalClassfinalto OSGi-exported classesFinalizePrivateFieldsfinalto fields TestNG writes reflectivelyAbstractClassPublicConstructorReplaceLambdaWithMethodReferenceSimplifyBooleanExpressionOrderImportsThere is no japicmp/revapi gate protecting the exported
org.testng.*packages, so the API- andreflection-affecting recipes are disqualifying on their own.
Java8toJava11is rejected separately:it would add JAXB, JAX-WS,
javax.injectandjavax.annotationruntime dependencies to alibrary that deliberately has almost none.
Also rejected:
rewrite-testing-frameworks(its only TestNG recipe migrates TestNG off its ownAPI),
OwaspTopTen(pulls in Spring, Jackson, servlet — none used), Gradle/dependency-updaterecipes (Dependabot covers this), and anything needing Java 15+/16+ since the target stays Java 11.
Making OpenRewrite and Autostyle agree
OpenRewrite auto-detects a formatting style when none is declared, and here it infers a layout that
groups
java.*/javax.*separately — not what google-java-format produces. Declaring anImportLayoutStylefixes it:OrderImportsoversrc/mainorg.testng.build.ImportLayoutSo import ordering is not a reason to avoid the composites — it is configurable, and configured.
SimplifyBooleanExpressionstays out, deliberatelyIt rewrites
!(a <= b)toa > b, which is not equivalent forNaN: every comparison against NaNis false, so the negated form fails an assertion as intended while the
>form silently passes.The assertion code that made this concrete —
AssertJUnit.assertEquals(double, double, double)andAssert.areEqual— has since moved to the separateorg.testng:testng-assertsartifact, and adry-run today shows the recipe touching only two benign
isEmpty()ternaries. It stays excludedanyway: the gain is cosmetic, while enabling it would put a rewrite that is unsound on
floating-point comparisons behind a CI gate that rewrites new code automatically.
The CI gate
A dedicated
OpenRewritejob runsrewriteDryRunwithfailOnRewriteDryRun=true. The gate is theGradle task itself, so
./gradlew rewriteDryRun -PfailOnRewriteDryRun=truereproduces it exactly —unlike a shell check that only exists inside GitHub Actions.
It is its own job rather than a matrix row (the matrix produces 8 jobs; this would be 8× the cost
for identical output) and has no
needs:, so wall-clock impact is nil. Test compilation and ErrorProne are skipped — test sources are excluded from rewriting so compiling them feeds nothing into
the LST, and Error Prone is a javac plugin OpenRewrite never sees. Measured ~86s → ~41s locally,
with the gate still failing on drift.
Scope: main sources only
**/src/test/**is excluded. The test tree mixes real tests with fixture classes whose methodnames, declaration order and
finalize()presence are exactly what the surrounding tests assert on,and
testng-core/src/test/resources/testng.xmlreferences test classes by fully qualified name, soa moved class would silently drop tests instead of failing. Because no test file is touched, that
hazard is unreachable here:
Verification
./gradlew buildgreen: 12624 tests, 0 failures../gradlew rewriteDryRunproduces an empty patch — the recipes are idempotent and thecommitted config matches the committed sources.
src/mainis modified except the config, workflow and docs this PR adds.skipAutostylefix verified both ways against a deliberate formatting violation.refactor:commit message.Non-goals
No Gradle or JDK changes, no Autostyle → Spotless swap, no CI rework beyond the added job.
rewriteRunis not wired intocheckorbuild.Summary by CodeRabbit