Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add failing test for CleanupMockitoImports #336

Conversation

pstreef
Copy link

@pstreef pstreef commented May 5, 2023

Recipe CleanupMockitoImports cleans too much. My argument matchers are being cleaned up while used. I have reproduced the problem with a new failing test.

I needed to add junit-pioneer as it was not yet present, I added with a fixed version as it looks like it is not part of the platform.

…tchers are being cleaned up while used. I have reproduced the problem with a new failing test.

I needed to add junit-pioneer as it was not yet present, I added with a fixed version as it looks like it is not part of the platform.
@timtebeek
Copy link
Contributor

Related to openrewrite/rewrite#3111

@pstreef
Copy link
Author

pstreef commented May 5, 2023

while I can "fix" this issue by adding a redundant "cast to self", I have not been able to reproduce this with lombok in these tests. That seems to be a secondary problem.

var acceptance = OfferAcceptance.builder().bookingId("bookingId").build(); //lombok builder
when(offersRepository.acceptOffer(any(),eq((OfferAcceptance) acceptance))).thenReturn(accepted);

keeps the import while

var acceptance = OfferAcceptance.builder().bookingId("bookingId").build(); //lombok builder
when(offersRepository.acceptOffer(any(),eq(acceptance))).thenReturn(accepted);

removes it

@timtebeek
Copy link
Contributor

Yes that also ties into openrewrite/rewrite#1297; full support for Lombok is a difficult one unfortunately. Let's hope we can add something quickly to at least take out the pain here.

Comment on lines +221 to +222
import lombok.RequiredArgsConstructor;
@RequiredArgsConstructor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These annotations are not supported in the input strings; not sure how it affects the test, but if we can replicate the issue without these that'd be better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import lombok.RequiredArgsConstructor;
@RequiredArgsConstructor

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like my issue is related to combination of lombok + var so that would make it hard to write a test to reproduce it :/

@pstreef
Copy link
Author

pstreef commented May 5, 2023

The following work:

var acceptance = OfferAcceptance.builder().bookingId("bookingId").build(); //lombok builder
when(offersRepository.acceptOffer(any(),eq((OfferAcceptance)acceptance))).thenReturn(accepted);
OfferAcceptance acceptance = OfferAcceptance.builder().bookingId("bookingId").build(); //lombok builder
when(offersRepository.acceptOffer(any(),eq(acceptance))).thenReturn(accepted);
var acceptance = OfferAcceptance.of"bookingId"); //non lombok static constructor
when(offersRepository.acceptOffer(any(),eq(acceptance))).thenReturn(accepted);
var acceptance = new OfferAcceptance("bookingId",null,null,null); //non lombok constructor
when(offersRepository.acceptOffer(any(),eq(acceptance))).thenReturn(accepted);

@timtebeek
Copy link
Contributor

timtebeek commented Jul 23, 2023

@pstreef I'm doubting a bit what to do about this PR; I think the underlying problem is mostly separately tracked in openrewrite/rewrite#3111. Would you be OK with closing this one in favor of that other one?

I'm also open to alternatives to create exceptions here, as the recipe does aim to not remove any imports when uncertain about types:

return "Removes unused `org.mockito` import symbols, unless its possible they are associated with method invocations having null or unknown type information.";

Perhaps org.openrewrite.java.NoMissingTypes could be added as a precondition here? That would certainly restrict when we make changes.

@pstreef
Copy link
Author

pstreef commented Jul 23, 2023

@timtebeek I'm fine with closing it as duplicate 👍

@pstreef pstreef closed this Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants