Remove try-catch-fail blocks from unit tests

[AlekSimpson]

What's changed?

This PR adds a new recipe to move exception to the method signature when there is a try catch block inside of a unit test method.

What's your motivation?

#216

Anything in particular you'd like reviewers to focus on?

I left a comment marking this but I see that the withThrows() method takes in a list of NameTree types. I can't seem to find how to get that NameTree type from the J.Catch block. I thought maybe the .getExceptions() method is what I should use but that returns an Expression. Is there maybe some way to convert that to a NameTree?

Anyone you would like to review specifically?

@timtebeek

Any additional context

RSPEC-3658

Checklist

• I've added unit tests to cover both positive and negative cases
• I've added the license header to any new files through ./gradlew licenseFormat
• I've used the IntelliJ auto-formatter on affected files
• I've updated the documentation (if applicable)

[sambsnyd]

JUnit reports assertion failures differently from uncaught exceptions.
Uncaught exceptions are considered a worse failure mode as they can indicate that the test itself failed to evaluate its criteria. So this could unintentionally increase the apparent severity of test failures.

In JUnit 5 the idiomatic way of dealing with a catch containing a fail would be to translate into Assertions.assertDoesNotThrow().

[AlekSimpson]

Assertions.assertDoesNotThrow()

So if I am understanding this correctly instead of getting rid of the try-catch block and changing the method signature, it would be better to instead change the Assert.fail() to Assertions.assertDoesNotThrow()?

[AlekSimpson]

Ok the latest changes almost complete the recipe. There are only two issues remaining; The first being that the ASSERT_FAIL_MATCHER does not match Assert.fail() and just always returns false. I suspect maybe I am missing something in the classpath resources because I am not sure why else this matcher would not be working. The second issue is that for some reason the recipe does not add the import statement for Assertions.doesNotThrow(). Maybe this also has something to do with the classpath resources, I am not sure.

[knutwannheden]

@AlekSimpson I added a few hints which should allow you to make progress.

[AlekSimpson]

@knutwannheden, Ok I made those changes you suggested. One thing though that I am still not sure about here is that two of the tests are not passing because its throwing an error when it reaches the JavaTemplate.builder() call in the J.Try visitor. It says: "Expected a template that would generate exactly one statement to replace one statement, but generated 2." I haven't seen something like this when using a template builder.

[knutwannheden]

Looking at the code I would probably get rid of the visitMethodDeclaration() method entirely and also the cursor messaging. I understand that it is there to make sure that we only modify code in tests. But I think if anybody is using JUnit in non test code then they can simply not run this recipe over that code.

I am also saying this because I know that test code is frequently made more modular by factoring out some assertions into helper methods. And then these would not get refactored. So I suggest we remove the matchers and leave the visitTry() only. But let's also see if @timtebeek has any good arguments against this simplification.

Meanwhile I will look at your template problem. It could be a bug in the template engine.

[timtebeek]

I am also saying this because I know that test code is frequently made more modular by factoring out some assertions into helper methods. And then these would not get refactored. So I suggest we remove the matchers and leave the visitTry() only. But let's also see if @timtebeek has any good arguments against this simplification.

Skimmed through a bit, but I'm fine replacing any try { ..1.. } catch ( ..2.. ) { fail(..3..); } anywhere, inside or outside tests, with Assertions.assertDoesNotThrow(() -> { ..1.. }).

I do wonder a bit if we should do anything about the potential information loss in ..2.. and ..3..; in the current setup we lose ..2.. and ..3...

-                      try {
-                          int divide = 50 / 0;
-                      }catch (ArithmeticException e) {
-                          Assert.fail(e.getMessage());
-                      }
+                      Assertions.assertDoesNotThrow(() -> {
+                          int divide = 50 / 0;
+                      });

Perhaps we should adopt either of these to retain the failure message? Which then can't use the exception anymore(!).
assertDoesNotThrow​(Executable executable, String message)
assertDoesNotThrow​(Executable executable, Supplier<String> messageSupplier)

[AlekSimpson]

I am also saying this because I know that test code is frequently made more modular by factoring out some assertions into helper methods. And then these would not get refactored. So I suggest we remove the matchers and leave the visitTry() only. But let's also see if @timtebeek has any good arguments against this simplification.

Skimmed through a bit, but I'm fine replacing any try { ..1.. } catch ( ..2.. ) { fail(..3..); } anywhere, inside or outside tests, with Assertions.assertDoesNotThrow(() -> { ..1.. }).

I do wonder a bit if we should do anything about the potential information loss in ..2.. and ..3..; in the current setup we lose ..2.. and ..3...

-                      try {
-                          int divide = 50 / 0;
-                      }catch (ArithmeticException e) {
-                          Assert.fail(e.getMessage());
-                      }
+                      Assertions.assertDoesNotThrow(() -> {
+                          int divide = 50 / 0;
+                      });

Perhaps we should adopt either of these to retain the failure message? Which then can't use the exception anymore(!). assertDoesNotThrow​(Executable executable, String message) assertDoesNotThrow​(Executable executable, Supplier<String> messageSupplier)

Ya I've been wondering about that too. Also try-catch blocks could have multiple catch blocks I believe. Something like try { ..1.. } catch ( ..2.. ) { fail(..3..); } catch ( ..4.. ) { fail(..5..); }

[AlekSimpson]

@knutwannheden when you get a chance to look into the possible template engine issue, could you let me know how that goes?

[knutwannheden]

I just had another look. With the current code I cannot detect any exceptions thrown by the test. So it is hard to determine if there is indeed a bug or just a usage error.

Regarding the discussion with the catch blocks: I think we should maybe instead migrate to AssertJ here, as it offers more options which don't lose these details. For example:

assertThatCode(() -> {
  throw new RuntimeException("e");
}).isNotInstanceOf(IllegalArgumentException.class);

or when we don't want any exception:

assertThatCode(() -> {
  throw new RuntimeException("e");
}).doesNotThrowAnyException();

or for when the code is expected to throw an exception and we want to make an assertion about it:

assertThatThrownBy(() -> {
  throw new RuntimeException("e");
}).isInstanceOf(RuntimeException.class);

[AlekSimpson]

@knutwannheden I know that when replacing the body of a method with a new body you would use the .replaceBody() method when passing in the coordinates to the .apply() method. Could the issue be related to that? I see that J.Try only has a .replace() method.

[knutwannheden]

@AlekSimpson Since you in your case replace the entire try and not just its body, I think the replace() coordinate should be fine. But there are indeed not coordinates for everything and if we find something that would be good to have, we can add a new coordinate for that in rewrite.

[AlekSimpson]

Ok the template error issue is fixed, I just needed to add .contextSensitive() to the template. The tests all pass now.

[timtebeek]

• We seem to imply here that we only ever look at the first catch block;
  I'd appreciate a test that uses two catch blocks, and appropriate handling (skip?)
• Have you given any thought to try-with-resources blocks? We would not want to remove those.
  Might be good to add in a test that uses try (Foo bar) { ... } catch (Exception e) { fail(); }

[AlekSimpson]

Yes I am not sure how we should handle try statements with multiple catch blocks. Did you see Knut's comment about using AssertJ instead? What are your thoughts on that?

[timtebeek]

There might be value in having a separate recipe for AssertJ, but not everyone is ready or willing to use that just yet. To me it then makes the most sense to check here that there is:

1. only one catch block, such that we know it's safe to apply this recipe
2. only one statement in the catch block, which is a fail(), with no or a simple String argument. We would not want to convert for instance fail(cleanUpAndReturnMessage()) might still have side effects that we don't want to remove.

[timtebeek]

Hmm; considering number two above: Assert.fail(e.getMessage()) should be fine.. It's not easy thinking of each possible scenario 😅

[AlekSimpson]

There might be value in having a separate recipe for AssertJ, but not everyone is ready or willing to use that just yet. To me it then makes the most sense to check here that there is:

1. only one catch block, such that we know it's safe to apply this recipe

2. only one statement in the catch block, which is a `fail()`, with no or a simple String argument. We would not want to convert for instance `fail(cleanUpAndReturnMessage())` might still have side effects that we don't want to remove.

Latest commit addresses this

[timtebeek]

There's been a bit of an unfortunate oversight earlier in the use of JUnit 4 Assert, whereas we're more likely to execute this recipe after a JUnit 5 migration, as part of the best practice recipes. Could you update the PR once more to reflect this change?

[AlekSimpson]

Ok @timtebeek let me know how these new changes look.