slachiewicz opened a new pull request, #840:
URL: https://github.com/apache/maven-shade-plugin/pull/840
Moves 18 test classes to Jupiter. The bulk is the OpenRewrite
`JUnit4to5Migration` recipe, but three things needed hand work — the third is
the one worth reviewing.
**`TransformerTesterRule` was a custom `TestRule`.** The recipe converted
the `@Test` annotations in `PropertiesTransformerTest` to Jupiter but left the
`@Rule` field alone. That still compiles, which is the dangerous part: under
Jupiter the rule would simply never run, and the four spec-driven tests would
have passed while asserting nothing. It is rewritten as an
`InvocationInterceptor` and applied with `@ExtendWith` instead of a `@Rule`
field.
One behavioural note on that rewrite: the JUnit 4 rule deliberately never
called `base.evaluate()` when a `@TransformerTest` spec was present — the test
body never ran. Jupiter requires `invocation.proceed()` to be called exactly
once, so the interceptor now does. That is safe here only because all four
`@TransformerTest` methods have empty bodies (`{}`), and
`PropertiesTransformerTest` is the only class using the annotation. Worth
knowing if anyone later adds a body to one of them.
**`MinijarFilterTest`** — the recipe emitted non-compiling code, turning
`TemporaryFolder.builder().assureDeletion().build()` into `File.builder()...`.
Replaced with a plain `@TempDir File` field.
**`DefaultShaderTest`** — `@ClassRule TemporaryFolder TEMPORARY_FOLDER`
became `@TempDir public static File TEMPORARY_FOLDER`, which trips two
checkstyle rules at once: `VisibilityModifier` exempts `@Rule`/`@ClassRule` but
not `@TempDir`, and `StaticVariableName` wants lowerCamelCase for non-final
statics. Renamed to a package-private `static File temporaryFolder`.
**Left on JUnit 4 deliberately: `ShadeMojoTest`.** It extends
`AbstractMojoTestCase` → `PlexusTestCase` → `junit.framework.TestCase`, with
methods discovered by the `test*` naming convention. That inheritance lives in
maven-plugin-testing-harness, not here. `junit:junit` therefore stays (the
harness marks it `optional`, so it must be declared explicitly), and
`junit-vintage-engine` is added so the class stays discoverable once surefire
switches to the platform provider.
`org.hamcrest:hamcrest` also stays — several tests use
`CoreMatchers`/`MatcherAssert` directly. `junit-jupiter-engine` is deliberately
not declared, since surefire provisions it.
`src/it` and `src/test/resources` are untouched — confirmed by diffing the
OpenRewrite output so that only `pom.xml` and `src/test/java` differ.
### Verification
`mvn test` before: `Tests run: 72, Failures: 0, Errors: 0, Skipped: 0`
(surefire selecting `JUnit4Provider`).
`mvn test` after: `Tests run: 72, Failures: 0, Errors: 0, Skipped: 0`
(surefire selecting `JUnitPlatformProvider`, with `ShadeMojoTest`'s 5 tests
running via the vintage engine).
`checkstyle:check` 0 violations; `spotless:check` clean.
Because "the tests still pass" proves nothing about an extension that might
silently do nothing, the interceptor was also checked negatively: corrupting
one expected value in `PropertiesTransformerTest` produces a real
`AssertionFailedError`, so it is genuinely enforcing.
Draft until CI confirms.
Generated-by: Claude Opus 5 (1M context)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
