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 test that generated invalid code #331

Merged
merged 4 commits into from
Apr 19, 2023

Conversation

Meijuh
Copy link
Contributor

@Meijuh Meijuh commented Apr 15, 2023

This PR adds a unit test assertThrowsAssignment that generates invalid code. In JUnit assertThrows returns the exception that is thrown, while in AssertJ there is no way (currently) to obtain the exception that is thrown.

The purpose of this PR is check whether it is desired behavior open rewrite generates invalid Java code. A solution would be to skip rewriting cases where assertThrows is used as RHS in an assignment, but that would leave JUnit assertions in code bases.

@timtebeek
Copy link
Contributor

Thanks for adding a test to reproduce an issue! Indeed best to not convert such cases if there's no direct replacement in AssertJ. If you add @ExpectedToFail we can already merge your PR as is; or if you'd like to attempt to fix the issue too, you're more than welcome to have a go. Whatever you prefer.

@Meijuh
Copy link
Contributor Author

Meijuh commented Apr 15, 2023

I'll push a fix that leaves such a case as is. Shouldn't be too hard to check if the assertThrows appears on the RHS of an assignment.

@timtebeek timtebeek added the bug Something isn't working label Apr 17, 2023
@Meijuh
Copy link
Contributor Author

Meijuh commented Apr 18, 2023

Instead of blacklisting the case where assertThrows appears on the RHS of an assignment, I decided to whitelist the case where assertThrows appears immediately inside a J.Block. This is to ensure the degenerate test case I added (and more like it) would not fail.

Also, I am unsure what exactly the code style is, where can I find it? Is the style I used okay?

@timtebeek
Copy link
Contributor

Instead of blacklisting the case where assertThrows appears on the RHS of an assignment, I decided to whitelist the case where assertThrows appears immediately inside a J.Block. This is to ensure the degenerate test case I added (and more like it) would not fail.

Looks like a fine addition to me; thanks for taking the time to think through other limitations as well, and providing a fix.

Also, I am unsure what exactly the code style is, where can I find it? Is the style I used okay?

We typically use the IntelliJ default, sometimes supplemented with .editorconfig.

I like the changes you've made! My only change is adding the Issue annotation with links here, such that we can refer to this discussion.

@timtebeek timtebeek merged commit b86ddfd into openrewrite:main Apr 19, 2023
@timtebeek
Copy link
Contributor

Thanks a lot! While we wait for the for the next release in a week or two you can already use our snapshot versions.

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.

2 participants