[jonico]

Fixes item 1 of #1227 (Surefire forkMode -> forkCount/reuseForks).

Per discussion on #1227, this lives here next to the other Surefire/Failsafe recipes (AddSurefireFailsafeArgLine, AddMockitoJavaAgentToMavenSurefirePlugin) rather than in rewrite-static-analysis, for the same reason those do: it's Maven-plugin-configuration surgery, not a static-analysis/code-style concern.

What it does

Surefire 3 dropped support for the deprecated forkMode parameter — it's silently ignored rather than honored, so a build that still sets it gets different (and undocumented) fork behaviour after upgrading, with no warning. This recipe migrates it to the documented modern equivalent, per the Surefire fork options docs:

forkMode	becomes
never	forkCount=0
once	forkCount=1, reuseForks=true
always	forkCount=1, reuseForks=false
perthread (with threadCount=N)	forkCount=N, reuseForks=false

Applies to both maven-surefire-plugin and maven-failsafe-plugin. A perthread config with no paired threadCount, or any value outside those four, is left untouched rather than guessed at. If forkCount/reuseForks are already present alongside forkMode, only the now-dead forkMode tag is removed (Surefire 3 already ignores it in that case).

Validation

I wasn't able to run the module's own Gradle test suite locally — compileJava here needs rewrite-static-analysis, and that specific artifact returns 403 Forbidden against CodeGenome with my account regardless of version (release or snapshot), while core rewrite-core/rewrite-java/rewrite-maven resolve fine. Account-level entitlement gap, not a build config issue, so I validated independently instead: a minimal standalone Gradle project depending only on the public rewrite-core/rewrite-xml/rewrite-maven/rewrite-test artifacts (8.56.1, Maven Central), with just these two new files copied in. All 8 tests pass there:

alwaysOnFailsafe()                                PASSED
perthreadWithoutThreadCountIsLeftUntouched()      PASSED
redundantForkModeAlongsideForkCountIsRemoved()    PASSED
once()                                            PASSED
never()                                           PASSED
unrelatedPluginIsLeftUntouched()                  PASSED
unrecognizedValueIsLeftUntouched()                PASSED
perthreadWithThreadCount()                        PASSED

8 tests, 0 failures, 0 errors

Happy to split this into its own tracking issue if that's still wanted, or adjust the mapping/placement if I've read the discussion wrong.

Generated with Claude Code

[jonico]

Both CI failures here are pre-existing and unrelated to this change:

• ExtractExplicitConstructorInvocationArgumentsTest (13 cases) — already tracked in Preserve Java 25 constructor extraction statements #1206, which documents the same recipe producing broken code (references to undeclared variables) specifically on Java 25; that test class is gated to run only on Java 25+, which is why it surfaces in the test (Java 25) job here.
• EhcacheJavaxToJakartaTest > importedBomManagedDependencyGetsExplicitJakartaVersion() — unrelated recipe, untouched by this diff.

MigrateSurefireFailsafeForkModeTest's own 8 tests pass in both CI runs. Should go green once #1206 lands or on a rerun against a non-Java-25 job.

[timtebeek]

Thanks so much for the larger summary, and contributing this fix here; I've triggered some automated scans and had a look through myself; I'd say we're close to getting this in. 🙏🏻

[timtebeek]

Thanks a lot for picking this up @jonico, and for the detailed write-up on #1227! I've pushed a few changes; listing them here mostly for any next PRs:

1. forkMode now takes precedence over an existing forkCount / reuseForks. Up to 3.0.0-M7, Surefire's convertDeprecatedForkMode overwrote those two with the values implied by a non-once forkMode. So forkMode=always next to forkCount=2 really ran one fork per test class. Only dropping forkMode there would have introduced exactly the silent change this recipe sets out to prevent. The values implied by always/never/perthread now overwrite existing ones; once and the perthread reuseForks default only fill in what is missing.
2. Also migrate forkMode inside <executions><execution><configuration>, which is common for Failsafe, plus the legacy none and pertest aliases Surefire still accepted (they map to never and always). pertest was used as the "unrecognized" value in the tests, so that test now uses a truly invalid value.
3. Run it as part of UpgradePluginsForJava25, right after the Surefire/Failsafe bump to 3.5.x. Otherwise folks would have to seek out this recipe and run it on their own, which isn't realistic at scale. A test there covers the combination.
4. Tests: dropped the mavenProject(..) wrapper and the unused <parallel> from the inputs, and grouped the no-change cases in a @Nested class NoChange.
5. Regenerated recipes.csv with ./gradlew recipeCsvGenerate for the new description and composite counts.

I left two questions inline where I'd like your take.

[timtebeek]

Curious what you think here: the docs table maps perthread to reuseForks=false, but from 2.14 up to 3.0.0-M7 convertDeprecatedForkMode only set forkCount = threadCount and left reuseForks at its default of true. So for anyone coming from 2.14+ (most builds today, I think), adding reuseForks=false changes behaviour, and test runs could get noticeably slower with a fresh JVM per class. Would it not make more sense to leave reuseForks alone for perthread, to match what those builds actually ran? I kept your mapping as-is for now, only making it not overwrite an explicit reuseForks.

[jonico]

@timtebeek I'll go with leaving reuseForks untouched for perthread as it's consistent with the rest of the recipe already preferring "what actually ran" over "what's written down" (e.g. forkMode overriding an existing forkCount/reuseForks, which mirrors the old convertDeprecatedForkMode behavior directly). Pushed the change plus a test proving an explicit existing reuseForks still survives untouched.

[timtebeek]

I wonder if we should guard against plugin versions before 2.14, which don't know forkCount/reuseForks at all. That includes a plugin without an explicit <version> on Maven before 3.9, where the default lifecycle binds Surefire 2.12.4. There we'd remove a forkMode that was still honoured, and the new parameters would be ignored. Within UpgradePluginsForJava25 the version gets bumped to 3.5.x right before, but only when one is declared. (Not checked how common this is.) Perhaps only migrate when the effective plugin version is absent from the POM or at least 2.14? Not a blocker from my side, but curious what you think.

[jonico]

@timtebeek This is what happened in the real migration #1227 is about. My project ccfmaster's original pom had:

<artifactId>maven-surefire-plugin</artifactId>
<version>2.8.1</version>
...
<forkMode>always</forkMode>

2.8.1 predates 2.14 by several years. If this recipe had run standalone against that pom, without the version bump happening in the same pass, it would have deleted a forkMode that 2.8.1 genuinely honored and replaced it with forkCount/reuseForks that 2.8.1 silently ignores. The reason why it worked for me is that the version bump (2.8.1 → 3.5.2) and the forkMode fix happened together, in the same change.

Your proposed condition (migrate when the effective version is absent or at least 2.14, skip otherwise) handles this real case correctly: My original pom had an explicit pre-2.14 version, so it correctly falls into "skip."

I pushed the changes along your proposal as well added test cases and ran successfully with my project, let me know what you think.

[jonico]

Rebased onto current main to clear the merge conflict (unrelated churn in java-version-25.yml/recipes.csv from other merged PRs). Also addressed both inline questions from the review:

• Left reuseForks untouched for perthread rather than setting false, matching what Surefire's own convertDeprecatedForkMode (2.14–3.0.0-M7) actually did, not the documented table where the two disagree.
• Added the version guard: skips migration when the plugin has an explicit version below 2.14 (which predates forkCount/reuseForks and would silently ignore them) or an unparseable one, migrates when absent or ≥2.14. Verified against ccfmaster's actual pre-migration pom from Feedback from a 152k-line Java 8 to 25 migration: the silent behavioural changes that cost the most, and which have no recipe #1227 (surefire 2.8.1 + forkMode=always) — confirms it's now left untouched there.

@timtebeek whenever you get a chance to take another look 🙏

[jonico]

Heads up: main's churn in java-version-25.yml/recipes.csv reintroduced the merge conflict since my last update — rebased onto current main again, tests still green. Mergeable as of now, whenever you have a chance @timtebeek 🙏