Skip to content

Commit

Permalink
Remove exemption for tests from EmptyCatch
Browse files Browse the repository at this point in the history
The style guide is being updated in unknown commit.

Using `assertThrows` style assertions is now preferred over using try/catch statements for expected exception tests.

Startblock:
  * unknown commit is submitted
PiperOrigin-RevId: 673511513
  • Loading branch information
cushon authored and Error Prone Team committed Sep 13, 2024
1 parent 350815a commit e855cc8
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.CatchTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.CatchTree;

Expand All @@ -47,12 +46,6 @@ public Description matchCatch(CatchTree tree, VisitorState state) {
if (state.getTokensForNode(block).stream().anyMatch(t -> !t.comments().isEmpty())) {
return NO_MATCH;
}
if (ASTHelpers.isJUnitTestCode(state)) {
return NO_MATCH;
}
if (ASTHelpers.isTestNgTestCode(state)) {
return NO_MATCH;
}
return describeMatch(tree);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ public class SomeTest {
public void testNG() {
try {
System.err.println();
// BUG: Diagnostic contains:
} catch (Exception doNotCare) {
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,8 @@

package com.google.errorprone.bugpatterns;

import static org.junit.Assert.fail;

import java.io.FileNotFoundException;
import org.junit.Test;

/**
* @author [email protected] (Ding Yuan)
Expand Down Expand Up @@ -126,13 +124,4 @@ public void catchIsLoggedOnly() {
System.out.println("Caught an exception: " + t);
}
}

@Test
public void expectedException() {
try {
System.err.println();
fail();
} catch (Exception expected) {
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

package com.google.errorprone.bugpatterns;

import static org.junit.Assert.fail;

import org.junit.Test;

/**
* @author [email protected] (Ding Yuan)
*/
Expand All @@ -32,4 +36,14 @@ public void catchIsCompleteEmpty() {

}
}

@Test
public void expectedException() {
try {
System.err.println();
fail();
// BUG: Diagnostic contains:
} catch (Exception expected) {
}
}
}
30 changes: 30 additions & 0 deletions docs/bugpattern/EmptyCatch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
The [Google Java Style Guide §6.2][style] states:

> It is very rarely correct to do nothing in response to a caught exception.
> (Typical responses are to log it, or if it is considered "impossible", rethrow
> it as an AssertionError.)
>
> When it truly is appropriate to take no action whatsoever in a catch block,
> the reason this is justified is explained in a comment.
When writing tests that expect an exception to be thrown, prefer using
[`Assert.assertThrows`][assertthrows] instead of writing a try-catch. That is,
prefer this:

```java
assertThrows(NoSuchElementException.class, () -> emptyStack.pop());
```

instead of this:

```java
try {
emptyStack.pop();
fail();
} catch (NoSuchElementException expected) {
}
```

[style]: https://google.github.io/styleguide/javaguide.html#s6.2-caught-exceptions

[assertthrows]: https://junit.org/junit4/javadoc/latest/org/junit/Assert.html#assertThrows(java.lang.Class,%20org.junit.function.ThrowingRunnable)

0 comments on commit e855cc8

Please sign in to comment.