Skip to content

Drop forced maybeAddImport(.., false) where templates already attribute the added types - #1234

Merged
timtebeek merged 2 commits into
mainfrom
tim/drop-forced-maybe-add-import
Oct 25, 2026
Merged

timtebeek merged 2 commits into
mainfrom
tim/drop-forced-maybe-add-import

Conversation

@timtebeek

Copy link
Copy Markdown
Member

maybeAddImport(fqn, false) adds the import even when nothing in the result references the type. It hides templates that produce untyped code, and it can leave an unused import behind when the recipe ends up not inserting the code. Most of these calls are no longer needed: the templates already have a classpath, or the recipe sets the types itself (AssertThrowsOnLastStatement, JMockitBlockRewriter). The plain maybeAddImport keeps every import the tests expect.

This drops the false argument from 47 calls in 27 files. That covers the AssertJ JUnit*ToAssertThat recipes and JUnitFailToAssertJFail, the TestNG-to-AssertJ recipes, the JMockit rewriters, AssertThrowsOnLastStatement, UpdateTestAnnotation, ExpectedExceptionToAssertThrows, TruthAssertWithMessageToAssertJ and AnyStringToNullable.

MockitoWhenOnStaticToMockStatic needed two more changes for the MockedStatic import. The template that adds the MockedStatic<A> field now has the mockito-core-5 classpath. The recipe also requests the import after it adds the field, because the earlier AddImport ran before the field existed.

Skipped:

  • MockitoWhenOnStaticToMockStatic keeps the forced import of the stubbed class. That class is usually a project type, so the template parser can't resolve it, and mockStatic(A.class) stays untyped. Setting the identifier types by hand only exposes the missing mockStatic method type. This is the same gap the test's identifiers(false) relaxation covers.
  • AssertToAssertions keeps its forced wildcard Assertions.* import. It swaps an existing wildcard import, and missingTypeInfo() checks that this still happens when the code doesn't type-check.
  • WhiteboxToReflectionVisitor is unchanged, as its forced imports have a documented reason.

Verified with ./gradlew test on the assertj, testng, jmockit, truth, mockito, junit5, junit6, junit and hamcrest test packages. Those packages hold the touched recipes' tests and the JUnit-to-AssertJ, TestNG-to-AssertJ, JMockit-to-Mockito, Mockito and JUnit 4-to-5 migration tests. The first run (1589 tests) showed 23 failures, all in MockitoWhenOnStaticToMockStaticTest and AssertToAssertionsTest. Those are the spots fixed or kept above, and both classes now pass along with the other packages (769 tests, 0 failures).

Fixes item N2 of #1211.

@timtebeek

Copy link
Copy Markdown
Member Author

I checked whether dropping false is safe when the assertion arguments use customer types that the templates' classpath doesn't have, both here and together with #1232.

What happens to the types. A context-free template compiles assertThat(__P__.<com.acme.Order>p()) against just assertj-core-3, so Order doesn't resolve there. The migrated call then comes back with a degenerate method type rather than null:

assertThat -> org.assertj.core.api.Assertions{name=assertThat,return=Unknown,parameters=[]}
isEqualTo  -> null

AddImport looks for references by declaring type, and that survives because the templates name the owner through .staticImports(..). So the import is still added. The degraded type information was already there before this PR, since these AssertJ templates were already context-free. This PR only changes what happens when no reference is found at all.

Scratch tests. Customer types came from a separate source file (Order, List<Order>, the nested Order.Line, a custom OrderException). After-type validation was off, and each test checked the printed result:

  • On this branch: JUnitAssertEqualsToAssertThat (plain, message and message-supplier forms), JUnitAssertTrueToAssertThat, TestNgAssertEqualsToAssertThat, UpdateTestAnnotation with @Test(expected = OrderException.class) and ExpectedExceptionToAssertThrows. All added assertThat or assertThrows. A type missing from the LST entirely (com.acme.Missing) kept its import too.
  • On Drop contextSensitive() from self-contained JavaTemplates #1232's branch: HamcrestMatcherToAssertJ (equalTo, and greaterThan on a Comparable customer type), HamcrestIsMatcherToAssertJ and AssertTrueEqualsToAssertEquals. All added their imports. The two PRs touch no files in common, and the Drop contextSensitive() from self-contained JavaTemplates #1232 recipes already used the referenced-only maybeAddImport after their now context-free templates.

The test classes for the recipes changed here, plus JUnit4to5Migration*, ran 411 tests with 0 failures and 8 skipped.

Remaining risk. An import would only go missing if a template produced the call with no declaring type at all, for example a template missing .staticImports(..) or the matching classpath. The existing tests catch that. Sam's report behind a530dbe was this kind of problem in UpdateTestAnnotation, but that template now passes #{any(java.lang.Class)} and #{any(org.junit.jupiter.api.function.Executable)} and attributed fine with a customer exception. I didn't test the Kotlin path with customer types.

Two comments still describe the forced import (in UpdateTestAnnotation and JMockitBlockRewriter). A follow-up commit removes them.

@timtebeek
timtebeek marked this pull request as ready for review October 25, 2026 13:34
@timtebeek
timtebeek merged commit a822daf into main Oct 25, 2026
2 checks passed
@timtebeek
timtebeek deleted the tim/drop-forced-maybe-add-import branch October 25, 2026 13:53
@github-project-automation github-project-automation Bot moved this from In Progress to Done in OpenRewrite Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

1 participant