Introduce `JUnitToAssertJRules` Refaster rule collection #417

giall · 2022-12-15T11:05:43Z

Related issue: #402

Left XXX comments on org.junit.jupiter.api.Assertions methods that do not (yet) have rules.

Decided to leave the equality-related ones for a follow-up PR to reduce the scope a little bit, as those also need a bit more work (equality with deltas, etc.).

assertEquals
assertArrayEquals
assertIterableEquals
assertNotEquals

The following methods don't seem to have direct AssertJ equivalents, but could also be handled later.

assertLinesMatch
assertAll
assertTimeout
assertTimeoutPreemptively

github-actions · 2022-12-15T11:07:57Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-15T11:09:38Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-15T11:48:42Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-15T16:32:59Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-19T09:04:01Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-20T07:17:00Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-20T12:21:04Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Added a few commits and rebased.

I'm slightly in doubt whether we should add Javadocs here @Stephan202.

For ordering parameters, should we adhere to the order in which they occur in the before or after template? We kind of use both options.

As we base most thinks on the content of the after template, it could make sense to do the same for this. This would also make more sense given that you can have multiple before templates (this also holds for aftertemplates technically speaking, but we don't do that within Picnic currently).

Not completely done with reviewing the PR but flushing my comments and considerations :).

rickie · 2022-12-20T07:13:38Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java

+final class JUnitToAssertJRules {
+  private JUnitToAssertJRules() {}
+
+  static final class Fail {


I see you used the TestNGToAssertJ class as input for the naming, which makes total sense. However that class is not completely up-to-date with our current naming scheme. Instead of using the before template we now use the content of the after template to decide on the name.

rickie · 2022-12-20T08:50:28Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java

+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    void after(boolean condition, Supplier<String> messageSupplier) {


I think that supplier would also suffice here. It's also used in the withFailMessage.

rickie · 2022-12-20T08:56:32Z

...b/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestOutput.java

+  @Override
+  public ImmutableSet<?> elidedTypesAndStaticImports() {
+    return ImmutableSet.of(
+        (Runnable) () -> assertDoesNotThrow(() -> null),


We don't need all these casts, just the first one it seems.

github-actions · 2022-12-20T12:27:34Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-27T06:14:50Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-27T07:52:57Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

The changes LGTM. Thanks for the PR @giall 🚀 ! Let's get rid of those type of assertions 😄!

Suggested commit message:

Introduce `JUnitToAssertJRules` Refaster rule collection (#417)

rickie · 2022-12-27T06:23:45Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java

+
+  static final class AssertThatWithFailMessageStringIsNotNull {
+    @BeforeTemplate
+    void before(Object object, String message) {


Further down we use actual and expected. In the before and after case the name of the parameter is called actual as well. So propose to change it here and in some of the other templates :).

(I see it's used quite inconsistently though).

Now did the same for the boolean related ones to make it consistent within the file.

(We plan to automate the naming of the parameter names and the order in which they should be defined.)

github-actions · 2022-12-27T08:06:34Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2022-12-29T07:56:07Z

Note: we should add Javadoc to the Refaster rules that are added.

Stephan202

Rebased and added a commit. Very nice work!

Also tweaked the suggested commit message a bit.

Stephan202 · 2023-01-01T09:23:34Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java

+final class JUnitToAssertJRules {
+  private JUnitToAssertJRules() {}
+
+  static final class ThrowNewAssertionError {


I wonder whether we should include New here, but let's defer that decision to "the big automated rename".

Stephan202 · 2023-01-01T09:31:52Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java

+    @AfterTemplate
+    @DoNotCall
+    void after() {
+      throw new AssertionError();


JUnit throws a subtype. There are many such differences; will add some text about this in the Javadoc.

Stephan202 · 2023-01-01T09:46:36Z

...ib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestInput.java

+        (Runnable) () -> assertDoesNotThrow(() -> null),
+        () -> assertFalse(true),
+        () -> assertInstanceOf(null, null),
+        () -> assertNotNull(null),
+        () -> assertNotSame(null, null),
+        () -> assertNull(null),
+        () -> assertSame(null, null),
+        () -> assertThrows(null, null),
+        () -> assertThrowsExactly(null, null),
+        () -> assertTrue(true),
+        () -> Assertions.fail());


Will push a suggestion to make this more in line with other tests :)

Stephan202 · 2023-01-01T09:55:57Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java

+  static final class FailWithMessage {
+    @BeforeTemplate
+    void before(String message) {
+      Assertions.fail(message);
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    void after(String message) {
+      fail(message);
+    }
+  }


Here and below: a number of these statements also return a value, and can thus be matched as expressions rather than statements. By not returning void here we match strictly more cases. (Cause Refaster expression templates can also match statements, but not vice versa.)

Edit: but in practice this is only useful for the fail methods, because with assertThat the return type is fundamentally different.

Stephan202 · 2023-01-01T10:06:48Z

...ib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestInput.java

+    Object actual = new Object();
+    Object expected = new Object();
+    assertSame(expected, actual);


We can also just use one-liners here :)

Stephan202 · 2023-01-01T10:18:02Z

...ib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestInput.java

+  void testAssertThatCodeDoesNotThrowAnyException() {
+    assertDoesNotThrow(() -> {});
+  }


We should test both cases.

Edit: and when I do, the resultant code fails to compile with error: incompatible types: lambda body is not compatible with a void functional interface for the conversion from assertDoesNotThrow(() -> "foo") to assertThatCode(() -> "foo").doesNotThrowAnyException(). This can be worked around by using something other than a constant expression, but is worth calling out in a comment.

Stephan202 · 2023-01-01T12:22:28Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java

+  static final class FailWithMessageAndThrowable {
+    @BeforeTemplate
+    void before(String message, Throwable throwable) {
+      Assertions.fail(message, throwable);
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    void after(String message, Throwable throwable) {
+      fail(message, throwable);
+    }
+  }


There's also a Throwable-only overload.

Stephan202 · 2023-01-01T12:50:46Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java

+  // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertEquals`.
+  // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertArrayEquals`.
+  // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertIterableEquals`.
+  // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertLinesMatch`.
+  // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertNotEquals`.


This listing isn't exhaustive (especially when we consider the various overloads), and making it so doesn't seem worth the hassle, if done manually. Let's call out the incompleteness at the start of the class, and defer further work to another PR.

I started work on this idea here (heavy WIP, and some changes should be moved to separate PRs, but the gist is there).

Wow, that branch is awesome! Looks like some nice changes @Stephan202 !

github-actions · 2023-01-01T13:00:49Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

giall force-pushed the giall/junit-to-assertj branch from db36cd7 to 45406fc Compare December 15, 2022 11:07

giall force-pushed the giall/junit-to-assertj branch from 45406fc to b8b093f Compare December 15, 2022 11:46

giall force-pushed the giall/junit-to-assertj branch from 7485fff to dc4122a Compare December 19, 2022 09:01

rickie self-requested a review December 20, 2022 07:10

rickie force-pushed the giall/junit-to-assertj branch from dc4122a to 372dac8 Compare December 20, 2022 07:12

rickie reviewed Dec 20, 2022

View reviewed changes

rickie force-pushed the giall/junit-to-assertj branch from 58dd6e6 to 398689b Compare December 27, 2022 06:12

rickie approved these changes Dec 27, 2022

View reviewed changes

rickie added the new feature label Dec 27, 2022

rickie added this to the 0.7.0 milestone Dec 27, 2022

rickie force-pushed the giall/junit-to-assertj branch from 35652c5 to c949d47 Compare December 27, 2022 08:04

rickie requested a review from Stephan202 December 29, 2022 07:55

giall added 6 commits January 1, 2023 10:13

Introduce JUnitToAssertJRules with first rule

3fd429b

Rewrite Assertions.fail and Assertions.assertFalse

ab6f209

Rewrite Assestions.assertNull and Assertions.assertNotNull

0b526ff

Rewrite Assertions.assertSame and Assertions.assertNotSame

609bed8

Rewrite exception-related assertions

1ec0972

Rewrite Assertions.assertInstanceOf

6557048

giall and others added 7 commits January 1, 2023 10:13

Remove JUnit assertions from illegal classes

aea03cc

Rename Refaster Rules

b1f31a9

Rename variables and slightly change tests

567050a

Remove XXX and comment something about it

d0bcf85

Rename part 1

182e120

s/runnable/throwingCallable/

030928a

Suggestions

6caccb4

Stephan202 force-pushed the giall/junit-to-assertj branch from c949d47 to 6caccb4 Compare January 1, 2023 12:58

Stephan202 approved these changes Jan 1, 2023

View reviewed changes

rickie changed the title ~~Rewrite JUnit assertions to AssertJ~~ Introduce JUnitToAssertJRules Refaster rule collection Jan 2, 2023

rickie merged commit e6e5071 into master Jan 2, 2023

rickie deleted the giall/junit-to-assertj branch January 2, 2023 07:51

oxkitsune mentioned this pull request May 8, 2023

Rewrite JUnit Assertions.assertThrows() to AssertJ assertThatThrownBy #402

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `JUnitToAssertJRules` Refaster rule collection #417

Introduce `JUnitToAssertJRules` Refaster rule collection #417

giall commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 19, 2022

github-actions bot commented Dec 20, 2022

github-actions bot commented Dec 20, 2022

rickie left a comment

rickie Dec 20, 2022

rickie Dec 20, 2022

rickie Dec 20, 2022

github-actions bot commented Dec 20, 2022

github-actions bot commented Dec 27, 2022

github-actions bot commented Dec 27, 2022

rickie left a comment •

edited by Stephan202

Loading

rickie Dec 27, 2022

rickie Dec 27, 2022

rickie Dec 27, 2022

github-actions bot commented Dec 27, 2022

rickie commented Dec 29, 2022 •

edited

Loading

Stephan202 left a comment

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

rickie Jan 2, 2023

github-actions bot commented Jan 1, 2023

Introduce JUnitToAssertJRules Refaster rule collection #417

Introduce JUnitToAssertJRules Refaster rule collection #417

Conversation

giall commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 19, 2022

github-actions bot commented Dec 20, 2022

github-actions bot commented Dec 20, 2022

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2022

github-actions bot commented Dec 27, 2022

github-actions bot commented Dec 27, 2022

rickie left a comment • edited by Stephan202 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 27, 2022

rickie commented Dec 29, 2022 • edited Loading

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 1, 2023

Introduce `JUnitToAssertJRules` Refaster rule collection #417

Introduce `JUnitToAssertJRules` Refaster rule collection #417

rickie left a comment •

edited by Stephan202

Loading

rickie commented Dec 29, 2022 •

edited

Loading