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

Support member references in AssertJ assertThrows #330

Merged
merged 1 commit into from
Apr 15, 2023

Conversation

Meijuh
Copy link
Contributor

@Meijuh Meijuh commented Apr 15, 2023

This PR adds support for rewriting JUnit assertions to AssertJ assertions for throwing member references.
Lambdas in the form of () -> { throw new Exception(); } were already supported. Now var::throwingMemberReference is also supported.

I'm not really sure what rewrite's default behavior is for unsupported semantics; the added code simply does not rewrite unsupported semantics. Alternatively, the PR can be adjusted to throw an IllegalArgumentException in the last if-else branch.

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Clear improvement to cover a case we had not identified before; thanks a lot!

@timtebeek
Copy link
Contributor

I'm not really sure what rewrite's default behavior is for unsupported semantics; the added code simply does not rewrite unsupported semantics. Alternatively, the PR can be adjusted to throw an IllegalArgumentException in the last if-else branch.

We strive to "do no harm", as you might have discovered in the docs already; that often means not making a change is preferred over making a partial change. Sometimes we add in a comment when we can not convert a particular case where conversion is necessary; in this case I think it's fine to leave in the original statement, as is implied by the fall through return mi;.

Throwing an IllegalArgumentException is typically undesirable; we want to allow users to confidently run recipes against large code bases; uncovered cases are ideally then best not converted rather than breaking a recipe run that might be chained into a larger composite recipe.

Thanks for bringing this case to our attention, and providing an immediate fix as well!

@timtebeek timtebeek merged commit a6cecb1 into openrewrite:main Apr 15, 2023
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.

None yet

2 participants