build: adopt OpenRewrite with a hand-curated recipe list

.github/CONTRIBUTING.md

@@ -99,3 +99,37 @@ exit $RESULT
 This will cause git to trigger the code styling automatically right before commiting your changes locally.
 
 After that you can quickly check if there were any formatting that was applied using `git status`
+
+### Automated refactoring with OpenRewrite
+
+The codebase makes use of [OpenRewrite](https://docs.openrewrite.org/) for automated refactoring.
+It is a maintenance tool you run on purpose; it is never part of `check` or `build`, so a normal
+build never invokes it.
+
+To see what the recipes would change without touching your working tree, run
+`./gradlew rewriteDryRun`. It writes a patch to `build/reports/rewrite/rewrite.patch`. **Read that
+patch before applying it** -- recipes are not always semantics-preserving, and at least one had to
+be dropped from our list because it silently broke a `NaN` comparison.
+
+To apply the recipes run `./gradlew rewriteRun`, and then **always** run `./gradlew autostyleApply`
+afterwards, as two separate commands:
+
+```bash
+./gradlew rewriteRun
+./gradlew autostyleApply
+```
+
+OpenRewrite does not emit google-java-format output, so Autostyle has to have the last word or
+`autostyleCheck` will fail. Do not combine the two into a single Gradle invocation: task ordering
+between them is not constrained and Gradle may interleave them.
+
+CI runs `./gradlew rewriteDryRun -PfailOnRewriteDryRun=true`, which fails when the recipes still
+have work to do. That exact command reproduces the check locally.
+
+The active recipes are listed in [`rewrite.yml`](../rewrite.yml) at the root of the repository. It
+is a hand-picked list rather than one of OpenRewrite's bundled composite recipes; the file explains
+why. If you want to add a recipe, add it there and include the dry-run patch in your pull request.
+
+**Note:** the OpenRewrite tasks are not compatible with Gradle's configuration cache
+([rewrite-gradle-plugin#366](https://github.com/openrewrite/rewrite-gradle-plugin/issues/366)). If
+you have it enabled, add `--no-configuration-cache` to the commands above.

.github/workflows/test.yml

@@ -37,6 +37,60 @@ jobs:
           npm ci --prefix .github/workflows
           node .github/workflows/matrix.mjs
 
+  rewrite:
+    name: 'OpenRewrite'
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+    steps:
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+        with:
+          persist-credentials: false
+          fetch-depth: 1
+      - name: Set up Java 25
+        uses: actions/setup-java@03ad4de0992f5dab5e18fcb136590ce7c4a0ac95 # v5
+        with:
+          java-version: 25
+          distribution: temurin
+          architecture: x64
+      # The gate is the Gradle task itself, so this is reproducible locally with
+      # ./gradlew rewriteDryRun -PfailOnRewriteDryRun=true
+      - name: Check the OpenRewrite recipes leave nothing to do
+        uses: burrunan/gradle-cache-action@4b67497abd37a511d6c1dc6299bdd84ff39f7bf5 # v3
+        with:
+          job-id: rewrite
+          # Test sources are excluded from rewriting, so compiling them feeds nothing into the
+          # LST -- skipping them is most of this job's runtime. Error Prone is a javac plugin
+          # that OpenRewrite never sees, and the build matrix already runs it.
+          arguments: |
+            --no-daemon
+            rewriteDryRun
+            -x compileTestJava
+            -x compileTestKotlin
+          properties: |
+            jdkBuildVersion=25
+            failOnRewriteDryRun=true
+            skipErrorProne=true
+            # rewriteDryRun builds a type-attributed LST of every main source file, which needs
+            # noticeably more heap than Gradle's default.
+            org.gradle.jvmargs=-Xmx3g
+            org.gradle.java.installations.auto-download=false
+      - name: Summarise the patch
+        if: ${{ failure() }}
+        run: |
+          patch=build/reports/rewrite/rewrite.patch
+          [ -s "$patch" ] || exit 0
+          echo "::error::OpenRewrite would still change these files. Run './gradlew rewriteRun', then './gradlew autostyleApply', review the diff and commit it."
+          git apply --stat "$patch"
+          echo "The full patch is attached as the 'rewrite-patch' artifact."
+      - name: Upload the patch
+        if: ${{ failure() }}
+        uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7
+        with:
+          name: rewrite-patch
+          path: build/reports/rewrite/rewrite.patch
+          if-no-files-found: ignore
+
   build:
     needs: matrix_prep
     runs-on: ${{ matrix.os }}

CHANGES.txt

@@ -1,4 +1,5 @@
 Current (7.13.0)
+New: Added OpenRewrite to the build with a hand-picked recipe list (see rewrite.yml), and applied it to the main sources (Julien Herr)
 Fixed: Remove leftover dead JUnit code: the deprecated unused ConversionUtils and orphaned JUnit test samples, following the removal of JUnit execution support in 7.10.0 (Julien Herr)
 Update: Dependency refresh: Guice 6.0.0, JCommander 2.0, snakeyaml 2.6, slf4j-api 2.0.18. Guice 7 and JCommander 3 were skipped: they require jakarta.inject and Java 17 respectively
 Fixed: GITHUB-3242: use-global-thread-pool no longer refuses to start a suite when the number of data-driven tests reaches thread-count (Krishnan Mahadevan)

build-logic/build-parameters/build.gradle.kts

@@ -51,6 +51,10 @@ buildParameters {
         defaultValue.set(true)
         description.set("Fail build on javadoc warnings")
     }
+    bool("failOnRewriteDryRun") {
+        defaultValue.set(false)
+        description.set("Fail rewriteDryRun when OpenRewrite still has changes to make")
+    }
     bool("skipErrorProne") {
         defaultValue.set(false)
         description.set("Skip Error Prone verifications")

build-logic/jvm/src/main/kotlin/testng.java.gradle.kts

@@ -4,7 +4,6 @@ plugins {
     `java-base`
     id("build-logic.build-params")
     id("testng.versioning")
-    id("testng.style")
     id("testng.repositories")
     // Improves Gradle Test logging
     // See https://github.com/vlsi/vlsi-release-plugins/tree/master/plugins/gradle-extensions-plugin
@@ -43,6 +42,10 @@ tasks.configureEach<Test> {
     inputs.property("java.vm.vendor", System.getProperty("java.vm.vendor"))
 }
 
+if (!buildParameters.skipAutostyle) {
+    apply(plugin = "testng.style")
+}
+
 if (!buildParameters.skipErrorProne) {
     apply(plugin = "testng.errorprone")
 }

build.gradle.kts

@@ -1,7 +1,44 @@
 plugins {
     id("testng.repositories")
+    id("build-logic.build-params")
     id("idea")
     id("com.gradleup.nmcp.aggregation") version "1.6.1"
+    id("org.openrewrite.rewrite") version "7.37.0"
+}
+
+dependencies {
+    rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:3.35.0"))
+    rewrite("org.openrewrite.recipe:rewrite-static-analysis")
+    rewrite("org.openrewrite.recipe:rewrite-migrate-java")
+}
+
+rewrite {
+    // The recipe list lives in rewrite.yml, which documents why the upstream composites
+    // (CommonStaticAnalysis, CodeCleanup, Java8toJava11) are not used directly.
+    activeRecipe("org.testng.build.ModernizeMainSources")
+
+    // Makes OpenRewrite lay imports out the way google-java-format does, so its output does not
+    // have to be corrected by autostyleApply. rewrite.yml explains the measurement behind this.
+    activeStyle("org.testng.build.ImportLayout")
+
+    // exclusion() matches file paths, not recipe names.
+    exclusion(
+        // Test sources are out of scope: test/** mixes real tests with fixture classes whose
+        // method names, declaration order and finalize() presence are what the surrounding
+        // tests assert on, and testng.xml references test classes by FQN.
+        "**/src/test/**",
+        // Nothing here rewrites build scripts, and .gradle.kts would be routed to the
+        // experimental Kotlin parser for no benefit. This also covers build-logic/ and
+        // build-logic-commons/, whose tracked files are all .kt or .gradle.kts.
+        "**/*.gradle.kts",
+        "**/*.kt",
+        "**/*.groovy",
+    )
+
+    // rewriteDryRun is a manual maintenance task by default. CI turns this on so the gate is
+    // the Gradle task itself, which means ./gradlew rewriteDryRun -PfailOnRewriteDryRun=true
+    // reproduces the CI check exactly.
+    failOnDryRunResults = buildParameters.failOnRewriteDryRun
 }
 
 val String.v: String get() = rootProject.extra["$this.version"] as String

rewrite.yml

@@ -0,0 +1,134 @@
+#
+# OpenRewrite recipe list for TestNG.
+#
+# The upstream composites (org.openrewrite.staticanalysis.CommonStaticAnalysis,
+# org.openrewrite.staticanalysis.CodeCleanup and org.openrewrite.java.migrate.Java8toJava11)
+# are deliberately NOT activated. OpenRewrite has no mechanism to exclude a single sub-recipe
+# from a composite -- exclusion() filters file paths only, and the upstream request to support
+# recipe exclusion (openrewrite/rewrite#1714) is closed as won't-fix.
+#
+# Measured on src/main with the import layout below active, CommonStaticAnalysis touches 101
+# files. Roughly half of that overlaps with the list below and is wanted; the rest comes from
+# recipes we must not run:
+#
+#   ExplicitInitialization          46   field-declaration churn, no benefit
+#   FinalClass                      12   adds final to OSGi-exported classes
+#   FinalizePrivateFields           10   adds final to fields TestNG writes reflectively
+#   AbstractClassPublicConstructor   6   narrows constructors on OSGi-exported classes
+#   ReplaceLambdaWithMethodReference 4   changes synthetic class shape
+#   SimplifyBooleanExpression        2   unsound on float comparisons, see the note further down
+#   OrderImports                     1   fights google-java-format; see the style block below
+#
+# There is no binary-compatibility gate (no japicmp/revapi) protecting the exported
+# org.testng.* packages, and TestNG resolves members reflectively, so the API- and
+# reflection-affecting recipes are disqualifying on their own. Java8toJava11 is rejected
+# separately: it would add JAXB, JAX-WS, javax.inject and javax.annotation runtime
+# dependencies and rewrite the Java version in .github/workflows/*.yml.
+#
+# So the sub-recipes we want are copied out by hand below.
+#
+# Scope: src/main only. See the exclusion() block in build.gradle.kts.
+#
+---
+# Import layout, declared so OpenRewrite agrees with the google-java-format output that
+# Autostyle enforces: static imports first, one blank line, then everything else in a single
+# ASCII-sorted block, never folded into star imports. This matches .editorconfig's
+# ij_java_imports_layout = $*,|,* and its two import-on-demand thresholds (lines 34-35).
+#
+# This is not cosmetic. OpenRewrite otherwise auto-detects a layout from the sources and infers
+# one that groups java.*/javax.* separately, which disagrees with google-java-format: measured
+# over src/main, OrderImports rewrites 34 files under auto-detection and 1 with this style active.
+# Declaring it keeps rewrite output already-formatted, so autostyleApply only has to tidy up, and
+# it keeps rewriteDryRun honest as an idempotency check.
+type: specs.openrewrite.org/v1beta/style
+name: org.testng.build.ImportLayout
+styleConfigs:
+  - org.openrewrite.java.style.ImportLayoutStyle:
+      classCountToUseStarImport: 999
+      nameCountToUseStarImport: 999
+      layout:
+        - 'import static all other imports'
+        - '<blank line>'
+        - 'import all other imports'
+
+---
+type: specs.openrewrite.org/v1beta/recipe
+name: org.testng.build.ModernizeMainSources
+displayName: TestNG main-source modernization
+description: >-
+  Hand-picked sub-recipes, grouped below from least to most intrusive. Recipes that matched
+  nothing are kept so a future contribution is covered by the same list.
+recipeList:
+  # --- Syntactic normalization. No intended behaviour change. ---
+  - org.openrewrite.staticanalysis.NeedBraces
+  - org.openrewrite.staticanalysis.UnnecessaryParentheses
+  - org.openrewrite.staticanalysis.MultipleVariableDeclarations
+  - org.openrewrite.staticanalysis.ModifierOrder
+  - org.openrewrite.staticanalysis.RemoveExtraSemicolons
+  - org.openrewrite.staticanalysis.UseDiamondOperator
+  - org.openrewrite.staticanalysis.UnnecessaryExplicitTypeArguments
+  - org.openrewrite.staticanalysis.UseJavaStyleArrayDeclarations
+  - org.openrewrite.staticanalysis.UpperCaseLiteralSuffixes
+  - org.openrewrite.staticanalysis.WriteOctalValuesAsDecimal
+
+  # --- Semantics-preserving library-call modernization. Includes the source-only leaves of
+  #     Java8toJava11; its build-file and dependency-adding halves are omitted. ---
+  # String handling
+  - org.openrewrite.staticanalysis.EqualsAvoidsNull
+  - org.openrewrite.staticanalysis.StringLiteralEquality
+  - org.openrewrite.staticanalysis.NoToStringOnStringType
+  - org.openrewrite.staticanalysis.NoValueOfOnStringType
+  - org.openrewrite.staticanalysis.ReplaceStringConcatenationWithStringValueOf
+  - org.openrewrite.staticanalysis.CaseInsensitiveComparisonsDoNotChangeCase
+  - org.openrewrite.staticanalysis.IndexOfReplaceableByContains
+  - org.openrewrite.staticanalysis.IndexOfChecksShouldUseAStartPosition
+  - org.openrewrite.staticanalysis.IndexOfShouldNotCompareGreaterThanZero
+  # StringBuilder
+  - org.openrewrite.staticanalysis.ChainStringBuilderAppendCalls
+  - org.openrewrite.staticanalysis.NewStringBuilderBufferWithCharArgument
+  - org.openrewrite.staticanalysis.ReplaceStringBuilderWithString
+  # Collections
+  - org.openrewrite.staticanalysis.IsEmptyCallOnCollections
+  - org.openrewrite.staticanalysis.CollectionToArrayShouldHaveProperType
+  - org.openrewrite.staticanalysis.NoDoubleBraceInitialization
+  - org.openrewrite.staticanalysis.NoEmptyCollectionWithRawType
+  - org.openrewrite.staticanalysis.SimplifyArraysAsList
+  - org.openrewrite.staticanalysis.UseMapContainsKey
+  # Boxing / numerics
+  - org.openrewrite.staticanalysis.NoPrimitiveWrappersForToStringOrCompareTo
+  - org.openrewrite.staticanalysis.PrimitiveWrapperClassConstructorToValueOf
+  - org.openrewrite.staticanalysis.BigDecimalDoubleConstructorRecipe
+  - org.openrewrite.staticanalysis.AtomicPrimitiveEqualsUsesGet
+  # Misc
+  - org.openrewrite.staticanalysis.RedundantFileCreation
+  - org.openrewrite.staticanalysis.RemoveRedundantNullCheckBeforeInstanceof
+  - org.openrewrite.staticanalysis.RemoveRedundantNullCheckBeforeLiteralEquals
+  # Java 8 -> Java 11 API equivalents
+  - org.openrewrite.java.migrate.nio.file.PathsGetToPathOf
+  - org.openrewrite.java.migrate.nio.file.RedundantUtf8Charset
+  - org.openrewrite.java.migrate.lang.JavaLangAPIs
+  - org.openrewrite.java.migrate.net.JavaNetAPIs
+  - org.openrewrite.java.migrate.concurrent.JavaConcurrentAPIs
+  - org.openrewrite.java.migrate.util.OptionalNotPresentToIsEmpty
+  - org.openrewrite.java.migrate.util.OptionalNotEmptyToIsPresent
+  - org.openrewrite.java.migrate.util.UsePredicateNot
+  - org.openrewrite.java.migrate.UseJavaUtilBase64
+  - org.openrewrite.java.migrate.CastArraysAsListToList
+
+  # --- Boolean and control-flow simplification. Read these hunks, do not skim them. ---
+  #
+  # SimplifyBooleanExpression and BooleanChecksNotInverted are deliberately NOT enabled: both
+  # rewrite `!(a <= b)` to `a > b`, which is not equivalent for NaN, where every comparison is
+  # 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)
+  # and Assert.areEqual -- has since moved to the separate org.testng:testng-asserts artifact, and
+  # a dry-run today shows SimplifyBooleanExpression touching only two benign isEmpty() ternaries.
+  # They stay excluded anyway: the gain is cosmetic, while enabling them would put a rewrite that
+  # is unsound on floating-point comparisons behind a CI gate that rewrites new code automatically.
+  - org.openrewrite.staticanalysis.SimplifyBooleanReturn
+  - org.openrewrite.staticanalysis.NoRedundantJumpStatements
+  - org.openrewrite.staticanalysis.UnnecessaryReturnAsLastStatement
+  - org.openrewrite.staticanalysis.InlineVariable
+  - org.openrewrite.staticanalysis.NoEqualityInForCondition
+  - org.openrewrite.staticanalysis.ForLoopIncrementInUpdate

testng-collections/src/main/java/org/testng/collections/Objects.java

@@ -11,7 +11,7 @@
     private final String m_name;
     private final String m_value;
 
     public ValueHolder(String name, String value) {
       m_name = name;
       m_value = value;
     }
@@ -25,7 +25,7 @@
       return m_name + "=" + m_value;
     }
 
     public boolean isEmptyString() {
       return Strings.isNullOrEmpty(m_value);
     }
   }
@@ -69,8 +69,12 @@
       StringBuilder result = new StringBuilder("[" + m_className + " ");
       for (int i = 0; i < values.size(); i++) {
         ValueHolder vh = values.get(i);
-        if (m_omitNulls && vh.isNull()) continue;
-        if (m_omitEmptyStrings && vh.isEmptyString()) continue;
+        if (m_omitNulls && vh.isNull()) {
+          continue;
+        }
+        if (m_omitEmptyStrings && vh.isEmptyString()) {
+          continue;
+        }
 
         if (i > 0) {
           result.append(" ");

testng-core-api/src/main/java/org/testng/internal/AutoCloseableLock.java

@@ -28,8 +28,12 @@ public void close() {
 
   @Override
   public boolean equals(Object object) {
-    if (this == object) return true;
-    if (object == null || getClass() != object.getClass()) return false;
+    if (this == object) {
+      return true;
+    }
+    if (object == null || getClass() != object.getClass()) {
+      return false;
+    }
     AutoCloseableLock that = (AutoCloseableLock) object;
     return Objects.equals(internalLock, that.internalLock);
   }

testng-core-api/src/main/java/org/testng/internal/ConstructorOrMethod.java

@@ -86,8 +86,11 @@ public boolean getEnabled() {
 
   @Override
   public String toString() {
-    if (m_method != null) return m_method.toString();
-    else return m_constructor.toString();
+    if (m_method != null) {
+      return m_method.toString();
+    } else {
+      return m_constructor.toString();
+    }
   }
 
   public String stringifyParameterTypes() {

testng-core-api/src/main/java/org/testng/internal/KeyAwareAutoCloseableLock.java

@@ -37,8 +37,12 @@ public void close() {
 
     @Override
     public boolean equals(Object object) {
-      if (this == object) return true;
-      if (object == null || getClass() != object.getClass()) return false;
+      if (this == object) {
+        return true;
+      }
+      if (object == null || getClass() != object.getClass()) {
+        return false;
+      }
       AutoReleasable that = (AutoReleasable) object;
       return Objects.equals(lock, that.lock);
     }

testng-core-api/src/main/java/org/testng/internal/Utils.java

@@ -69,8 +69,7 @@ public static void setVerbose(int n) {
   public static void writeUtf8File(
       @Nullable String outputDir, String fileName, XMLStringBuffer xsb, String prefix) {
     try {
-      final File outDir =
-          (outputDir != null) ? new File(outputDir) : new File("").getAbsoluteFile();
+      final File outDir = outputDir != null ? new File(outputDir) : new File("").getAbsoluteFile();
       if (!outDir.exists()) {
         boolean ignored = outDir.mkdirs();
       }