JUnit 5 Assertions.assertThrows should only contain a single statement

[timtebeek]

Hi! Noticed a somewhat surprising pattern after migrating from JUnit 4 to JUnit 5, which is captured in the unit test assertThrowsMultiLine:

@Test
fun assertThrowsMultiLine() = assertChanged(
    before = """
        import org.junit.Test;
        
        public class A {
        
            @Test(expected = IllegalArgumentException.class)
            public void test() {
                String foo = "foo";
                throw new IllegalArgumentException("boom");
            }
        }
    """,
    after = """
        import org.junit.jupiter.api.Test;
        
        import static org.junit.jupiter.api.Assertions.assertThrows;
        
        public class A {
        
            @Test
            void test() {
                assertThrows(IllegalArgumentException.class, () -> {
                    String foo = "foo";
                    throw new IllegalArgumentException("boom");
                });
            }
        }
    """
)

While technically equivalent to the JUnit 4 model, where any of the two statements might throw an exception caught in the assertion, it's nonetheless perhaps too broad, particularly when there are more than a handful of statements, all of which might throw an exception. Ideally, in my opinion, one would only assert that the last statement is used within the assertThrows invocation.

@Test
void test() {
    String foo = "foo";
    assertThrows(IllegalArgumentException.class, () -> throw new IllegalArgumentException("boom"));
}

[timtebeek]

Generalized, and separated from the JUnit 4 to 5 migration, this could even be a recipe in and of itself:

package org.openrewrite.java.testing.junit5

import org.junit.jupiter.api.Test
import org.openrewrite.java.JavaRecipeTest

class AssertThrowsOnSingleStatementTest : JavaRecipeTest {

    override val recipe = AssertThrowsOnSingleStatement()

    @Test
    fun assertThrowsOnSingleStatement() = assertChanged(
        before = """
            package org.openrewrite.java.testing.junit5;
            
            import static org.junit.jupiter.api.Assertions.assertThrows;
            
            public class SimpleAssertThrowsTest {
            
                public void throwsExceptionWithSpecificType() {
                    assertThrows(NullPointerException.class, () -> {
                        Object foo = null;
                        foo.equals("");
                        throw new NullPointerException();
                    });
                }
            }
        """,
        after = """
            package org.openrewrite.java.testing.junit5;
            
            import static org.junit.jupiter.api.Assertions.assertThrows;
            
            public class SimpleAssertThrowsTest {
            
                public void throwsExceptionWithSpecificType() {
                    Object foo = null;
                    foo.equals("");
                    assertThrows(NullPointerException.class, () -> throw new NullPointerException());
                }
            }
        """
    )
}

That way any errors in the test setup, or further changes, are picked up correctly rather than silently swallowed, with unreachable code in the assertion lambda. In the before example above the throw statement is never reached, as foo.equals("") already also(!) throws a NPE, which is silently caught.

Now applying such a pattern to a large code base could uncover such previously swallowed exceptions or dead code, which could be surprising to users. That should put a generalized form more in the best practices range of recipes, maybe not best lumped in with composed recipe sets.

[timtebeek]

I briefly looked at implementing such a visitor myself, based off of the similar AssertJ JUnitAssertThrowsToAssertExceptionType Recipe. However I failed to see how best to split the statements within the lambda body into those that should come before the assertThrows, and the final one that should be inside the assertThrows.

Would such a Recipe be a worthwhile addition to the existing set, before I spend more time on it?

[jkschneider]

Yes, it would be worthwhile. I love the idea of narrowing the method to the place where the exception is thrown. Unfortunately JUnit's assertThrows returns void. So if the method invocation that throws an exception is used in a variable initializer, assignment, or assignment operation then you can't wrap that whole statement.

// won't work
String s = assertThrows(SomeException.class, methodThatThrowsAndReturnsString());
assertThat(s).isEqualTo("tim");

AssertJ does have a:

String s = assertThatThrownBy(methodThatThrowsAndReturnsString()).isInstance(SomeException.class);
assertThat(s).isEqualTo("tim");

We can check the JavaSourceSet marker on the root J.CompilationUnit to see if AssertJ happens to be on the classpath in this project. Hamcrest might have similar?

[timtebeek]

Not sure I fully follow the problematic usage examples you've given, as I'm seeing slightly different method signatures:

// JUnit 5
public static <T extends Throwable> T assertThrows(Class<T> expectedType, Executable executable) {
  return AssertThrows.assertThrows(expectedType, executable);
}

// AssertJ
public static AbstractThrowableAssert<?, ? extends Throwable> assertThatThrownBy(ThrowingCallable shouldRaiseThrowable) {
  return assertThat(catchThrowable(shouldRaiseThrowable)).hasBeenThrown();
}

// Nothing for Hamcrest

So the JUnit assertThrows returns the thrown exception, and AssertJ's assertThatThrownBy returns a wrapper for further assertions. Both response types are disconnected from the statements within the argument Executable or ThrowingCallable, as they return a response based off the exception class.

So if we take a slightly longer before situation as our example:

import org.junit.jupiter.api.Test;

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

class A {
  @Test
  void test() {
    IndexOutOfBoundsException ioobe = assertThrows(IndexOutOfBoundsException.class, () -> {
      int[] arr = {};
      int first = arr[0];
    });
    assertEquals("Index 0 out of bounds for length 0", ioobe.getMessage());
  }
}

Then we have both an assignment statement within the Executable, and an assignment of the assertThrows response.

I would expect that this could still be rewritten to an after situation such as:

import org.junit.jupiter.api.Test;

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

class A {
  @Test
  void test() {
    int[] arr = {};
    IndexOutOfBoundsException ioobe = assertThrows(IndexOutOfBoundsException.class, () -> {
      int first = arr[0];
    });
    assertEquals("Index 0 out of bounds for length 0", ioobe.getMessage());
  }
}

Not how the Executable retains the assignment within (for now), and assertThrows response is left in place. Creating the array however is pulled up and out of the Executable, such that there can be no ambiguities around which statement should throw the Exception.

Now granted, the results aren't spectacular here, but imagine there's 10 statements within the Executable, and you have a lot more chance of ending up with dead test code if either of those earlier statements can throw the same exception. The idea is then to extract all but the final statement from the Executable, even if it's a variable assignment, and place those above the assertThrows statement, taking into account any assignment there as well.

It would seem to me that should be possible with the current testing and rewrite framework offerings; would you agree there? Or am I still missing something?

[tkvangorder]

Converting from JUnit 4 -> JUnit 5: if a JUnit 4 test has multiple lines (and the same exception could potentially be thrown in multiple places) I am inclined to say that we should not attempt to narrow where the exception is thrown. The goal of the migration is to have parity between the versions and have a "do no harm" mentality. We want the users to trust the migration.

HOWEVER, I do like the idea of having a new JUnit 5 cleanup recipe that is specifically focused on narrowing where the exception is thrown. I think we might need data flow analysis to do this correctly though.

Consider something like this:

import org.junit.jupiter.api.Test;

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

class A {
  @Test
  void test() {
    IndexOutOfBoundsException ioobe = assertThrows(IndexOutOfBoundsException.class, () -> {
        int[] arr = {};
        int index = 0;
        index++;
        int first = arr[index];
      });
    assertEquals("Index 0 out of bounds for length 0", ioobe.getMessage());
  }
}

If we narrow this to the last statement, this will result in a compiler error because index is not effectively final.

import org.junit.jupiter.api.Test;

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

class A {
  @Test
  void test() {
    int[] arr = {};
    int index = 0;
    index++;
    IndexOutOfBoundsException ioobe = assertThrows(IndexOutOfBoundsException.class, () -> {
        int first = arr[index];
      });
    assertEquals("Index 0 out of bounds for length 0", ioobe.getMessage());
  }
}

So we can only safely do this by checking that any references to variables are final or effectively final.

Another tricky use case would be if there are multiple assertThrows in the same test and the lambda expressions both define the same variable:

import org.junit.jupiter.api.Test;

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

class A {
  @Test
  void test() {
    IndexOutOfBoundsException ioobe = assertThrows(IndexOutOfBoundsException.class, () -> {
        int[] arr = {};
        int first = arr[0];
      });
    assertEquals("Index 0 out of bounds for length 0", ioobe.getMessage());

    ioobe = assertThrows(IndexOutOfBoundsException.class, () -> {
        int[] arr = {1};
        int first = arr[1];
      });
    assertEquals("Index 1 out of bounds for length 1", ioobe.getMessage());
  }
}

import org.junit.jupiter.api.Test;

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

class A {
  @Test
  void test() {
    int[] arr = {};
    IndexOutOfBoundsException ioobe = assertThrows(IndexOutOfBoundsException.class, () -> {
        int first = arr[0];
      });
    assertEquals("Index 0 out of bounds for length 0", ioobe.getMessage());

    int[] arr = {1};
    ioobe = assertThrows(IndexOutOfBoundsException.class, () -> {
        int first = arr[1];
      });
    assertEquals("Index 1 out of bounds for length 1", ioobe.getMessage());
  }
}

This can be quite tricky when considering these edge cases.

[timtebeek]

Oh wow, you're exactly right with those problematic use cases. Guess that makes the implementation more challenging. But at least good to know my suggestion came across, as it's indeed not to be executed as part of a 4 to 5 migration; I like the "do no harm" phrasing there.

[shivanisky]

I would prefer the recipe that breaks the build personally based on my experience running this recipe on a large codebase a few weeks ago. I got 3 complilation errors anyway. Perhaps it can be an option for the user which one they choose - by default not break the build, or optionally break the build when using it on last statement. And then can finetune the breaking the build one with abovementioned cases, as I think they are quite rare.

[shivanisky]

Would love to get this in, I'm happy to work on it, depending on my workload this week. If anyone else wants to take it, please go for it.

[shivanisky]

Can do this with following suggestion from @timtebeek

"I'm leaning mostly towards a separate recipe that can be run through best practices, not immediately part of a JUnit 4 to 5 migration by default. That way folks can be sure a migration will work, with best practices that might slightly change semantics ran separately."

[timtebeek]

Yes thanks for the offer to take this on! Probably best to start with unit tests to set out the constraints for what will and wont cover just yet. I'm thinking of tests with a single assertThrows as the only statement, where the last statement within the assertThrows block exclusively uses variables only assigned once. That ought to then cover at least some of the cases converted from JUnit 4 's expected exceptions to JUnit Jupiter.

Anything else we can ignore for now, such as to do no harm, while leaving the door open for future extensions such as

• also allow explicitly final variables with conditional assignments
• allow variables not explicitly marked final, but effectively final
• also allow statements before a single assertThrows
• allow multiple assertThrows provided there's no conflicting variables

Do stay mindful of your own time too though; I know you've contributed a lot on your own time already. 🙏🏻

[timtebeek]

• Fixed in https://github.com/openrewrite/rewrite-testing-frameworks/pull/590 🎉