-
Notifications
You must be signed in to change notification settings - Fork 78
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
Convert assertTrue(false, String)
and assertFalse(true, String)
with fail(String)
#558
Conversation
src/main/java/org/openrewrite/java/testing/cleanup/AssertFalseLiteralTrueToFail.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/cleanup/AssertTrueLiteralFalseToFail.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/cleanup/AssertFalseLiteralTrueToFailTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/cleanup/AssertFalseLiteralTrueToFailTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/cleanup/AssertFalseLiteralTrueToFailTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/cleanup/AssertTrueLiteralFalseToFailTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/cleanup/AssertTrueLiteralFalseToFailTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/cleanup/AssertTrueLiteralFalseToFailTest.java
Outdated
Show resolved
Hide resolved
Awesome start, thanks! Did you notice this comment in the issue?
I think we can keep the tests, and replace the recipes with just a couple lines in Refaster recipes. |
assertTrue(false, String)
and assertFalse(true, String)
with fail(String)
Must admit I skimmed over that part :) I will continue working on this next week and apply the proposed changes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see! Let's merge the two separate test classes into a single AssertLiteralBooleanToFailRecipeTest
, and within those I think we write a single test that covers all four cases from a single unit test that just has the four statements in one.
src/main/java/org/openrewrite/java/testing/cleanup/AssertLiteralBooleanToFail.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/cleanup/AssertFalseLiteralTrueToFailTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/cleanup/AssertTrueLiteralFalseToFailTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat addition; thanks @Laurens-W !
What's changed?
Add recipe to rewrite assertTrue(false, "message") and assertFalse(true, "message") to fail("message")
What's your motivation?
AssertTrue(false)
tofail
#464Anything in particular you'd like reviewers to focus on?
Test coverage
Anyone you would like to review specifically?
@timtebeek @timo-abele
Have you considered any alternatives or workarounds?
Any additional context
Checklist