openrewrite / rewrite-spring

OpenRewrite recipes for Spring projects.
Apache License 2.0
254 stars 75 forks source link

SpringBoot2JUnit4to5Migration: JUnit4 assertions are not correctly updated to JUnit5. #171

Closed josemariavillar closed 2 years ago

josemariavillar commented 2 years ago

Good afternoon, I am having problems when applying the SpringBoot2JUnit4to5Migration recipe in the following Test:

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;

....

    @Test
    void assertionsTest() {
        assertNotNull(userRole.getId());
        assertEquals(role, userRole.getRole());
        assertEquals(user, userRole.getUser());

        Exception exception = assertThrows(NumberFormatException.class, new ThrowingRunnable() {
            public void run() throws Throwable {
                Integer.parseInt("1a");
            }
        });

        String expectedMessage = "For input string";
        String actualMessage = exception.getMessage();

        assertTrue(actualMessage.contains(expectedMessage));
        assertThat(actualMessage).isEqualTo(expectedMessage);
    }

When I run the mvn rewrite:dryRun or mvn rewrite:run command I would expect the "import static " to update correctly, instead the import of "assertEquals" and "assertNotNull" are removed and not replaced by their corresponding ones in Junit5 causing it to not compile the code, as can be seen in the following result:

--- "a/src\\test\\java\\com\\project\\service\\UserRoleTest.java"
+++ "b/src\\test\\java\\com\\project\\service\\UserRoleTest.java"
@@ -4,18 +4,16 @@ org.openrewrite.java.testing.junit5.AssertToAssertions, org.openrewrite.java.testing.junit5.UpdateBeforeAfterAnnotations, org.openrewrite.java.testing.junit5.UpdateTestAnnotation
 import com.project.model.User;
 import com.project.model.UserRole;
 import com.project.model.id.UserRoleKey;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.function.ThrowingRunnable;
+import org.junit.function.ThrowingRunnable;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 import org.mockito.Mockito;

 import java.sql.Timestamp;

 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertThrows;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;

 public class UserRoleTest {

@@ -28,7 +26,7 @@
     private final String CTE_CO_USER = "CTE_CO_USER";
     private final String CTE_CO_ROLE = "CTE_CO_ROLE";

-    @Before
+    @BeforeEach
     void setUp() {
         timestamp = Mockito.mock(Timestamp.class);

Attached is a test case where the problem can be reproduced and observed: https://github.com/josemariavillar/test_project/tree/assert_to_assertions

pway99 commented 2 years ago

Thanks @josemariavillar, I will have a look

pway99 commented 2 years ago

@josemariavillar, I could reproduce the issue; thanks so much for the test project.

FindMissingTypes is one of the first tools I use when investigating invalid recipe results. The results show several missing types. Looking at your project, I found that it's using Lombok, which is not supported at this time.

 user = /*~~(type is 'null')~~>*/User.builder().coUser(CTE_CO_USER)
                .coUserModification("codification")
                .tsModification(timestamp)
                .build();
        role = /*~~(type is 'null')~~>*/Role.builder().coRole(CTE_CO_ROLE)
                .coUserModification("codification")
                .tsModification(timestamp)
                .build();

        userRoleKey = /*~~(type is 'null')~~>*/UserRoleKey.builder().coUser(CTE_CO_USER).coRole(CTE_CO_ROLE)
                .build();

        userRole = /*~~(type is 'null')~~>*/UserRole.builder()
                .id(userRoleKey)
                .user(user)
                .role(role)
                .tsDelete(timestamp)
                .coUserModification("codification")
                .tsModification(timestamp)
                .build();
    }

    @Test
    void assertionsTest() {
        /*~~(type is 'null')~~>*/assertNotNull(userRole.getId());
        /*~~(type is 'null')~~>*/assertEquals(role, userRole.getRole());
        /*~~(type is 'null')~~>*/assertEquals(user, userRole.getUser());
pway99 commented 2 years ago

requires lombok support https://github.com/openrewrite/rewrite/issues/1297

josemariavillar commented 2 years ago

Thanks for the information @pway99. I have a doubt about what you comment, in this specific case what is affecting the lombok library? I don't understand it.

josemariavillar commented 2 years ago

Regardless, a possible solution while supporting Loombok is to add generically the "import static org.junit.jupiter.api.Assertions.*", thus avoiding any problems.

pway99 commented 2 years ago

Hi @josemariavillar,

I have a doubt about what you comment, in this specific case what is affecting the lombok library? I don't understand it.

Thanks for asking, and please accept my apology for closing this issue with such a brief explanation.

In this specific case, the UserRole object uses Lombok annotations, so the UserRole.getId() method is not a member of the source file parsed by the rewrite-java-parser when the plugin is executed.

As a result the assertNotNull(userRole.getId()) statement has a null method type.

image

Consequently, the Assertions.assertNotNull import is not added by the AddImportVisitor because its type does not exist on the source tree.

pway99 commented 2 years ago

Regardless, a possible solution while supporting Lombok is to add generically the "import static org.junit.jupiter.api.Assertions.*", thus avoiding any problems.

Unfortunately, other recipes such as OrderImports might unfold the stared import, and the original problem remains.

Thanks again for identifying these issues :)