openrewrite / rewrite-testing-frameworks

OpenRewrite recipes that perform common Java testing migration tasks.
Apache License 2.0
77 stars 73 forks source link

`JUnitAssertThrowsToAssertExceptionType` does not convert cases where `executable` is a variable #511

Open timtebeek opened 7 months ago

timtebeek commented 7 months ago

What version of OpenRewrite are you using?

What is the smallest, simplest way to reproduce the problem?

import org.junit.jupiter.api.function.Executable;

import static org.junit.jupiter.api.Assertions.assertThrows;

public class SimpleExpectedExceptionTest {
    public void throwsExceptionWithSpecificType() {
        Executable executable = () -> {
            throw new NullPointerException();
        };
        assertThrows(NullPointerException.class, executable);
    }
}

What did you expect to see?

import org.assertj.core.api.ThrowableAssert.ThrowingCallable;

import static org.junit.jupiter.api.Assertions.assertThrows;

public class SimpleExpectedExceptionTest {
    public void throwsExceptionWithSpecificType() {
        ThrowingCallable executable = () -> {
            throw new NullPointerException();
        };
        assertThatExceptionOfType(NullPointerException.class).isThrownBy(executable);
    }
}

What did you see instead?

No change, as the recipe fails to match the case where the argument is a variable. https://github.com/openrewrite/rewrite-testing-frameworks/blob/7ba17bbbfb2c9f5437a377d4e022c101a31579e2/src/main/java/org/openrewrite/java/testing/assertj/JUnitAssertThrowsToAssertExceptionType.java#L68-L74

Additional context

As reported on

Riyazul555 commented 5 months ago

I guess instead of setting executable to null if it’s not a Lambda or MemberReference, like this

J executable = mi.getArguments().get(1);
                if (executable instanceof J.Lambda) {
                    executable = ((J.Lambda) executable).withType(THROWING_CALLABLE_TYPE);
                } else if (executable instanceof J.MemberReference) {
                    executable = ((J.MemberReference) executable).withType(THROWING_CALLABLE_TYPE);
                } else {
                    executable = executable.withType(THROWING_CALLABLE_TYPE);
                }

the code now sets the type of executable directly. This allows variables to be used correctly. The withType(THROWING_CALLABLE_TYPE) method is applied to all types of Executable, ensuring the transformation applies universally and keeping the method signature and descriptions concise and relevant. This change should handle cases where the Executable is a variable in the assertThrows call.

What are your thoughts @timtebeek ??

timtebeek commented 3 months ago

We've had another look at this, but this seems harder than initially anticipated. We'd have to track assignments and change their type to make this work. Removing the good-first-issue label.