Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Findbugs #618

Merged
merged 11 commits into from
Aug 8, 2016
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ install:
- '[ "${TRAVIS_PULL_REQUEST}" = "false" ] && ./gradlew :ui:jfxNative --stacktrace || ./gradlew --stacktrace '

script:
- ./gradlew checkstyleMain checkstyleTest pmdMain jacocoTestReport jacocoRootReport test --stacktrace -Pheadless=true -PlogTests
- ./gradlew checkstyleMain checkstyleTest pmdMain findbugsMain jacocoTestReport jacocoRootReport test --stacktrace -Pheadless=true -PlogTests

after_success:
- if [[ "$TRAVIS_OS_NAME" != "osx" ]]; then codecov ; fi
Expand Down
2 changes: 1 addition & 1 deletion appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ build_script:

# to run your custom scripts instead of automatic tests
test_script:
- gradlew.bat checkstyleMain checkstyleTest pmdMain jacocoTestReport jacocoRootReport test --stacktrace -Pheadless=true -PlogTests
- gradlew.bat checkstyleMain checkstyleTest pmdMain findbugsMain jacocoTestReport jacocoRootReport test --stacktrace -Pheadless=true -PlogTests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this run on the tests too? We want to find possible bugs in the tests too right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JLLeitschuh Fixed


platform:
- x64
Expand Down
27 changes: 27 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import groovy.xml.XmlUtil

buildscript {
repositories {
jcenter()
Expand Down Expand Up @@ -92,6 +94,7 @@ configure(allprojects - project(':ui:linuxLauncher')) {
apply plugin: 'net.ltgt.errorprone'
apply plugin: 'checkstyle'
apply plugin: 'pmd'
apply plugin: 'findbugs'

configurations.errorprone {
resolutionStrategy.force 'com.google.errorprone:error_prone_core:2.0.9'
Expand Down Expand Up @@ -119,6 +122,29 @@ configure(allprojects - project(':ui:linuxLauncher')) {
ruleSets = []
}

findbugs {
sourceSets = [sourceSets.main]
excludeFilter = new File(rootDir, "findBugsSuppressions.xml")
effort = "max"
ignoreFailures = true // Error will be reported by checkFindBugsReport
}

task checkFindBugsReport << {
def xmlReport = findbugsMain.reports.getXml().destination
if (findbugsMain.reports.getXml().destination.exists()) {
def bugs = (new XmlParser().parse(xmlReport)).BugInstance
def bugsFound = bugs.size()
if (bugsFound > 0) {
bugs.each { System.out.println(new XmlUtil().serialize(it)) }
throw new GradleException("$bugsFound FindBugs rule violations were found. See the report at: $xmlReport")
}
}
}

tasks.withType(FindBugs) {
it.finalizedBy checkFindBugsReport
}

repositories {
mavenCentral()
jcenter()
Expand All @@ -129,6 +155,7 @@ configure(allprojects - project(':ui:linuxLauncher')) {


dependencies {
compile group: 'com.google.code.findbugs', name: 'annotations', version: '3.0.1'
testCompile group: 'net.jodah', name: 'concurrentunit', version: '0.4.2'
testCompile group: 'org.hamcrest', name: 'hamcrest-all', version: '1.3'
testCompile group: 'junit', name: 'junit', version: '4.12'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;

import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ public void perform() {
continue;
}

final double ratio = bb.width() / bb.height();
final double ratio = (double) bb.width() / (double) bb.height();
if (ratio < minRatio || ratio > maxRatio) {
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

import com.google.common.collect.ImmutableList;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import org.bytedeco.javacpp.BytePointer;
import org.bytedeco.javacpp.IntPointer;

Expand Down Expand Up @@ -133,6 +135,7 @@ public class PublishVideoOperation implements Operation {
};

@SuppressWarnings("JavadocMethod")
@SuppressFBWarnings("ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD") // Don't need sync in constructor
public PublishVideoOperation(InputSocket.Factory inputSocketFactory) {
if (numSteps != 0) {
throw new IllegalStateException("Only one instance of PublishVideoOperation may exist");
Expand All @@ -141,7 +144,7 @@ public PublishVideoOperation(InputSocket.Factory inputSocketFactory) {
false));
this.qualitySocket = inputSocketFactory.create(SocketHints.Inputs
.createNumberSliderSocketHint("Quality", 80, 0, 100));
numSteps = numSteps + 1;
numSteps++;

serverThread = new Thread(runServer, "Camera Server");
serverThread.setDaemon(true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

public class ValveOperation implements Operation {

public static OperationDescription DESCRIPTION =
public static final OperationDescription DESCRIPTION =
OperationDescription.builder()
.name("Valve")
.summary("Toggle an output socket on or off using a boolean")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import org.eclipse.jetty.server.Request;

import java.io.IOException;
Expand Down Expand Up @@ -69,6 +71,7 @@ public final class DataHandler extends PedanticHandler {
}

@Override
@SuppressFBWarnings("UW_UNCOND_WAIT") // Bug in FindBugs. There is a condition.
Copy link
Member

@JLLeitschuh JLLeitschuh Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's actually a field in the warning that you can fill out called "justification"
http://findbugs.sourceforge.net/api/edu/umd/cs/findbugs/annotations/SuppressFBWarnings.html#justification()
So you can do @SuppressFBWarnings("UW_UNCOND_WAIT", justification = "Bug in FindBugs. There is a condition.")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I updated the suppressions to use that.

protected void handleIfPassed(String target,
Request baseRequest,
HttpServletRequest request,
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/edu/wpi/grip/core/util/SafeShutdown.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package edu.wpi.grip.core.util;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import javax.annotation.Nullable;

/**
Expand All @@ -16,8 +18,10 @@ public final class SafeShutdown {
* flagging a shutdown
* that we can't control.
*/

Runtime.getRuntime().addShutdownHook(new Thread() {
@Override
@SuppressFBWarnings("ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD")
public void run() {
SafeShutdown.stopping = true;
}
Expand Down
11 changes: 11 additions & 0 deletions findBugsSuppressions.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<FindBugsFilter>
<Match>
<Package name="~edu\.wpi\.grip\.generated.*" />
</Match>
<Match>
<Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE" />
</Match>
<Match>
<Bug pattern="NP_METHOD_PARAMETER_TIGHTENS_ANNOTATION" />
</Match>
</FindBugsFilter>
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

import com.google.common.eventbus.Subscribe;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import javafx.application.Platform;
import javafx.geometry.Orientation;
import javafx.scene.control.CheckBox;
Expand Down Expand Up @@ -38,7 +40,8 @@ public class BlobsSocketPreviewView extends SocketPreviewView<BlobsReport> {
private final Mat tmp = new Mat();
private final GripPlatform platform;
@SuppressWarnings("PMD.ImmutableField")
private boolean showInputImage;
@SuppressFBWarnings("IS2_INCONSISTENT_SYNC") // No sync needed in constructor
private boolean showInputImage = false;

/**
* @param socket An output socket to preview.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

import com.google.common.eventbus.Subscribe;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import java.util.List;
import javafx.application.Platform;
import javafx.geometry.Orientation;
Expand Down Expand Up @@ -40,7 +42,8 @@ public class LinesSocketPreviewView extends SocketPreviewView<LinesReport> {
private final Mat tmp = new Mat();
private final GripPlatform platform;
@SuppressWarnings("PMD.ImmutableField")
private boolean showInputImage;
@SuppressFBWarnings("IS2_INCONSISTENT_SYNC") // No sync needed in constructor
private boolean showInputImage = false;

/**
* @param socket An output socket to preview.
Expand Down