Skip to content

Commit

Permalink
Handle ExpectedException tests that end with an explicit failure
Browse files Browse the repository at this point in the history 
This code may be written to ensure the test fails if the code under test
doesn't throw. It's unnecessary and will break the test if the code under
test is wrapped in assertThrows/expectThrows, so remove it.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=174253158
  • Loading branch information
@cushon @eaftan
cushon authored and eaftan committed Nov 2, 2017
1 parent 85afeb6 commit 9af93b6
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 7 deletions.
31 changes: 27 additions & 4 deletions core/src/main/java/com/google/errorprone/bugpatterns/AbstractExpectedExceptionChecker.java
View file
Expand Up @@ -19,13 +19,17 @@
import static com.google.common.collect.Iterables.getLast; import static com.google.common.collect.Iterables.getLast;
import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.anything;
import static com.google.errorprone.matchers.Matchers.expressionStatement; import static com.google.errorprone.matchers.Matchers.expressionStatement;
import static com.google.errorprone.matchers.Matchers.staticMethod; import static com.google.errorprone.matchers.Matchers.staticMethod;
import static com.google.errorprone.matchers.Matchers.throwStatement;
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
import static com.google.errorprone.util.ASTHelpers.getReceiver; import static com.google.errorprone.util.ASTHelpers.getReceiver;
import static com.google.errorprone.util.ASTHelpers.isSubtype; import static com.google.errorprone.util.ASTHelpers.isSubtype;


import com.google.common.base.Joiner; import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterators; import com.google.common.collect.Iterators;
import com.google.common.collect.PeekingIterator; import com.google.common.collect.PeekingIterator;
import com.google.errorprone.VisitorState; import com.google.errorprone.VisitorState;
Expand All @@ -50,9 +54,12 @@
import com.sun.tools.javac.code.Symtab; import com.sun.tools.javac.code.Symtab;
import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree;
import java.util.ArrayDeque;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Deque;
import java.util.List; import java.util.List;
import java.util.regex.Pattern; import java.util.regex.Pattern;
import javax.annotation.Nullable;


/** @author cushon@google.com (Liam Miller-Cushon) */ /** @author cushon@google.com (Liam Miller-Cushon) */
public abstract class AbstractExpectedExceptionChecker extends BugChecker public abstract class AbstractExpectedExceptionChecker extends BugChecker
Expand All @@ -69,6 +76,10 @@ public abstract class AbstractExpectedExceptionChecker extends BugChecker
.onClassAny("org.hamcrest.Matchers", "org.hamcrest.CoreMatchers", "org.hamcrest.core.Is") .onClassAny("org.hamcrest.Matchers", "org.hamcrest.CoreMatchers", "org.hamcrest.core.Is")
.withSignature("<T>isA(java.lang.Class<T>)"); .withSignature("<T>isA(java.lang.Class<T>)");


static final Matcher<StatementTree> FAIL_MATCHER =
anyOf(
throwStatement(anything()), expressionStatement(staticMethod().anyClass().named("fail")));

@Override @Override
public Description matchMethod(MethodTree tree, VisitorState state) { public Description matchMethod(MethodTree tree, VisitorState state) {
if (tree.getBody() == null) { if (tree.getBody() == null) {
Expand Down Expand Up @@ -103,9 +114,13 @@ Description scanBlock(MethodTree tree, BlockTree block, VisitorState state) {
if (expectations.isEmpty()) { if (expectations.isEmpty()) {
return NO_MATCH; return NO_MATCH;
} }
List<StatementTree> suffix = new ArrayList<>(); Deque<StatementTree> suffix = new ArrayDeque<>();
StatementTree failure = null;
Iterators.addAll(suffix, it); Iterators.addAll(suffix, it);
return handleMatch(tree, state, expectations, suffix); if (!suffix.isEmpty() && FAIL_MATCHER.matches(suffix.peekLast(), state)) {
failure = suffix.removeLast();
}
return handleMatch(tree, state, expectations, ImmutableList.copyOf(suffix), failure);
} }


/** /**
Expand All @@ -118,9 +133,14 @@ Description scanBlock(MethodTree tree, BlockTree block, VisitorState state) {
* @param suffix the statements after the assertions, which are expected to throw * @param suffix the statements after the assertions, which are expected to throw
*/ */
protected abstract Description handleMatch( protected abstract Description handleMatch(
MethodTree tree, VisitorState state, List<Tree> expectations, List<StatementTree> suffix); MethodTree tree,
VisitorState state,
List<Tree> expectations,
List<StatementTree> suffix,
@Nullable StatementTree failure);


protected BaseFix buildBaseFix(VisitorState state, List<Tree> expectations) { protected BaseFix buildBaseFix(
VisitorState state, List<Tree> expectations, @Nullable StatementTree failure) {
String exceptionClass = "Throwable"; String exceptionClass = "Throwable";
// additional assertions to perform on the captured exception (if any) // additional assertions to perform on the captured exception (if any)
List<String> newAsserts = new ArrayList<>(); List<String> newAsserts = new ArrayList<>();
Expand Down Expand Up @@ -197,6 +217,9 @@ protected BaseFix buildBaseFix(VisitorState state, List<Tree> expectations) {
((JCTree) expectations.get(0)).getStartPosition(), ((JCTree) expectations.get(0)).getStartPosition(),
state.getEndPosition(getLast(expectations)), state.getEndPosition(getLast(expectations)),
""); "");
if (failure != null) {
fix.delete(failure);
}
return new BaseFix(fix.build(), exceptionClass, newAsserts); return new BaseFix(fix.build(), exceptionClass, newAsserts);
} }


Expand Down
10 changes: 7 additions & 3 deletions core/src/main/java/com/google/errorprone/bugpatterns/ExpectedExceptionChecker.java
View file
Expand Up @@ -34,6 +34,7 @@
import com.sun.source.tree.StatementTree; import com.sun.source.tree.StatementTree;
import com.sun.source.tree.Tree; import com.sun.source.tree.Tree;
import java.util.List; import java.util.List;
import javax.annotation.Nullable;


/** @author cushon@google.com (Liam Miller-Cushon) */ /** @author cushon@google.com (Liam Miller-Cushon) */
@BugPattern( @BugPattern(
Expand All @@ -44,15 +45,18 @@
tags = StandardTags.FRAGILE_CODE tags = StandardTags.FRAGILE_CODE
) )
public class ExpectedExceptionChecker extends AbstractExpectedExceptionChecker { public class ExpectedExceptionChecker extends AbstractExpectedExceptionChecker {

@Override @Override
protected Description handleMatch( protected Description handleMatch(
MethodTree tree, VisitorState state, List<Tree> expectations, List<StatementTree> suffix) { MethodTree tree,
VisitorState state,
List<Tree> expectations,
List<StatementTree> suffix,
@Nullable StatementTree failure) {
if (suffix.size() <= 1) { if (suffix.size() <= 1) {
// for now, allow ExpectedException as long as it's testing that exactly one statement throws // for now, allow ExpectedException as long as it's testing that exactly one statement throws
return NO_MATCH; return NO_MATCH;
} }
BaseFix baseFix = buildBaseFix(state, expectations); BaseFix baseFix = buildBaseFix(state, expectations, failure);
// provide fixes to wrap each of the trailing statements in a lambda // provide fixes to wrap each of the trailing statements in a lambda
// skip statements that look like assertions // skip statements that look like assertions
ImmutableList<Fix> fixes = ImmutableList<Fix> fixes =
Expand Down
70 changes: 70 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/ExpectedExceptionCheckerTest.java
View file
Expand Up @@ -366,4 +366,74 @@ public void nothingButAsserts() throws IOException {
"}") "}")
.doTest(); .doTest();
} }

@Test
public void removeExplicitFail() throws IOException {
BugCheckerRefactoringTestHelper.newInstance(new ExpectedExceptionChecker(), getClass())
.addInputLines(
"in/ExceptionTest.java",
"import static com.google.common.truth.Truth.assertThat;",
"import static org.junit.Assert.fail;",
"import java.io.IOException;",
"import java.nio.file.*;",
"import org.junit.Test;",
"import org.junit.Rule;",
"import org.junit.rules.ExpectedException;",
"class ExceptionTest {",
" @Rule ExpectedException thrown = ExpectedException.none();",
" @Test",
" public void testThrow() throws Exception {",
" thrown.expect(IOException.class);",
" throw new IOException();",
" }",
" @Test",
" public void one() throws Exception {",
" Path p = Paths.get(\"NOSUCH\");",
" thrown.expect(IOException.class);",
" Files.readAllBytes(p);",
" assertThat(Files.exists(p)).isFalse();",
" fail();",
" }",
" @Test",
" public void two() throws Exception {",
" Path p = Paths.get(\"NOSUCH\");",
" thrown.expect(IOException.class);",
" Files.readAllBytes(p);",
" assertThat(Files.exists(p)).isFalse();",
" throw new AssertionError();",
" }",
"}")
.addOutputLines(
"out/ExceptionTest.java",
"import static com.google.common.truth.Truth.assertThat;",
"import static org.junit.Assert.assertThrows;",
"import static org.junit.Assert.fail;",
"",
"import java.io.IOException;",
"import java.nio.file.*;",
"import org.junit.Rule;",
"import org.junit.Test;",
"import org.junit.rules.ExpectedException;",
"class ExceptionTest {",
" @Rule ExpectedException thrown = ExpectedException.none();",
" @Test",
" public void testThrow() throws Exception {",
" thrown.expect(IOException.class);",
" throw new IOException();",
" }",
" @Test",
" public void one() throws Exception {",
" Path p = Paths.get(\"NOSUCH\");",
" assertThrows(IOException.class, () -> Files.readAllBytes(p));",
" assertThat(Files.exists(p)).isFalse();",
" }",
" @Test",
" public void two() throws Exception {",
" Path p = Paths.get(\"NOSUCH\");",
" assertThrows(IOException.class, () -> Files.readAllBytes(p));",
" assertThat(Files.exists(p)).isFalse();",
" }",
"}")
.doTest();
}
} }

0 comments on commit 9af93b6

Please sign in to comment.