Skip to content

Commit

Permalink
Implement issue 390
Browse files Browse the repository at this point in the history
  • Loading branch information
WarpspeedSCP authored and KengoTODA committed Aug 17, 2020
1 parent 186ad19 commit b4a884e
Show file tree
Hide file tree
Showing 6 changed files with 151 additions and 1 deletion.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
* [A meaningless exception data from `SAXBugCollectionHandler`](https://lgtm.com/projects/g/spotbugs/spotbugs/rev/a77ab08634687b7791e902636996ab6184462693)
* Use URI for files instead of converting string to URI each time. Fixes tests on Windows.

### Added
* Implement [issue 390](https://github.com/spotbugs/spotbugs/issues/390) as a detector, `DontAssertInstanceofInTests`, which reports bugs of type `JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS`.

## 4.1.1 - 2020-07-31
### Fixed
* Missing the version of commons-lang3 for Maven ([#1239](https://github.com/spotbugs/spotbugs/issues/1239))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package edu.umd.cs.findbugs.ba;

import edu.umd.cs.findbugs.AbstractIntegrationTest;
import edu.umd.cs.findbugs.BugInstance;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
import org.hamcrest.MatcherAssert;
import org.junit.Test;

import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;

public class Issue390Test extends AbstractIntegrationTest {

@Test
public void test() {
performAnalysis("ghIssues/Issue390.class");
BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder().bugType("JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS").build();
MatcherAssert.assertThat(getBugCollection(), containsExactly(1, bugTypeMatcher));
}

}
4 changes: 3 additions & 1 deletion spotbugs/etc/findbugs.xml
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,8 @@
reports="DM_DEFAULT_ENCODING"/>
<Detector class="edu.umd.cs.findbugs.detect.CheckRelaxingNullnessAnnotation" speed="fast"
reports="NP_METHOD_RETURN_RELAXING_ANNOTATION,NP_METHOD_PARAMETER_TIGHTENS_ANNOTATION"/>

<Detector class="edu.umd.cs.findbugs.detect.DontAssertInstanceofInTests" speed="fast"
reports="JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS"/>
<!-- Bug Categories -->
<BugCategory category="NOISE" hidden="true"/>

Expand Down Expand Up @@ -675,6 +676,7 @@
<BugCode abbrev="FB"/>
<BugCode abbrev="AT"/>
<!-- Bug patterns -->
<BugPattern abbrev="JUA" type="JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS" category="BAD_PRACTICE" experimental="true" />
<BugPattern abbrev="CN" type="OVERRIDING_METHODS_MUST_INVOKE_SUPER" category="CORRECTNESS" />
<BugPattern abbrev="CNT" type="CNT_ROUGH_CONSTANT_VALUE" category="BAD_PRACTICE"/>

Expand Down
34 changes: 34 additions & 0 deletions spotbugs/etc/messages.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1660,11 +1660,45 @@ factory pattern to create these objects.</p>
]]>
</Details>
</Detector>
<Detector class="edu.umd.cs.findbugs.detect.DontAssertInstanceofInTests">
<Details>
<![CDATA[
<p>Detector for patterns in JUnit tests where the type of an object
is checked by asserting the instanceof operator before an object
is cast to a certain type. </p>
<p>
This should be avoided as the ClassCastException that would result
from an improper cast may provide more information regarding the
cause of the error than a "false is not true" message which would
result from asserting the result of the instanceof operator.
</p>
<p>It is a fast detector</p>
]]>
</Details>
</Detector>
<!--
**********************************************************************
BugPatterns
**********************************************************************
-->

<BugPattern type="JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS">
<ShortDescription> Asserting value of instanceof in tests is not recommended. </ShortDescription>
<LongDescription> Assertion of type {0} in {2} at {3} may hide useful information about why a cast may have failed.</LongDescription>
<Details>
<![CDATA[
<p>Asserting type checks in tests is not recommended as a class cast exception message could better indicate
the cause of an instance of the wrong class being used than an instanceof assertion.</p>
<p>When debugging tests that fail due to bad casts, it may be more useful to observe the output of the
resulting ClassCastException which could provide information about the actual encountered type.
Asserting the type before casting would instead result in a less informative <code>"false is not true"</code>
message.</p>
]]>
</Details>
</BugPattern>

<BugPattern type="OVERRIDING_METHODS_MUST_INVOKE_SUPER">
<ShortDescription>Super method is annotated with @OverridingMethodsMustInvokeSuper, but the overriding method isn't calling the super method.</ShortDescription>
<LongDescription>Super method is annotated with @OverridingMethodsMustInvokeSuper, but {1} isn't calling the super method.</LongDescription>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package edu.umd.cs.findbugs.detect;

import edu.umd.cs.findbugs.BugInstance;
import edu.umd.cs.findbugs.BugReporter;
import edu.umd.cs.findbugs.BytecodeScanningDetector;

import org.apache.bcel.Const;
import org.apache.bcel.classfile.AnnotationEntry;
import org.apache.bcel.classfile.Code;
import org.apache.bcel.classfile.Method;

/**
* A detector that checks for lines in JUnit tests that look like `assertTrue(object instanceof Class)` and discourages them.
*
* It may be more useful to observe error messages from bad casts, as this may reveal more information regarding the
* circumstances of the exception than a "false is not true" assert message.
*
* This detector reports bugs of the type `JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS`.
*/
public class DontAssertInstanceofInTests extends BytecodeScanningDetector {
private boolean isTest;

private BugInstance currBug = null;

BugReporter bugReporter;

public DontAssertInstanceofInTests(BugReporter bugReporter) {
this.bugReporter = bugReporter;
}

@Override
public void visitMethod(Method obj) {
isTest = false;
for (AnnotationEntry entry : obj.getAnnotationEntries()) {
String entryType = entry.getAnnotationType();
isTest |= "Lorg/junit/Test;".equals(entryType);
}
}

@Override
public void visitCode(Code obj) {
if (isTest)
super.visitCode(obj);
}

@Override
public void sawOpcode(int seen) {

switch (seen) {
case Const.INSTANCEOF: {
// Initialise the bug instance here.
currBug = new BugInstance(this, "JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS", NORMAL_PRIORITY);
String operand = getDottedClassConstantOperand();
currBug.addTypeOfNamedClass(operand); // {0}

break;
}
case Const.INVOKESTATIC: {
if (getPrevOpcode(1) == Const.INSTANCEOF) {
String ciOp = getClassConstantOperand();
String ncOp = getNameConstantOperand();

if ("org/junit/Assert".equals(ciOp) &&
"assertTrue".equals(ncOp)) {
// This condition only triggers if the previous opcode was instanceof,
// so currBug is guaranteed to be a new BugInstance.
currBug.addClassAndMethod(this) // {2}
.addSourceLine(this); // {3}
bugReporter.reportBug(currBug);
}
}
}
}

}
}
14 changes: 14 additions & 0 deletions spotbugsTestCases/src/java/ghIssues/Issue390.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package ghIssues;

import org.junit.Test;

import static org.junit.Assert.assertTrue;

public class Issue390 {

@Test
public void test() {
Integer intVal = 1;
assertTrue(intVal instanceof Integer);
}
}

0 comments on commit b4a884e

Please sign in to comment.