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

feature: new option to analyze tests #720

Merged
merged 1 commit into from
Feb 17, 2023
Merged
Show file tree
Hide file tree
Changes from all 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: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ This option might be used to get around the `One (sub)project contains Java sour

**Only analyze** (`sonar.findbugs.onlyAnalyze`): Restrict analysis to a comma-separated list of classes and packages. For large projects, this may greatly reduce the amount of time needed to run the analysis. (However, some detectors may produce inaccurate results if they aren’t run on the entire application.) Classes should be specified using their full classnames (including package), and packages should be specified in the same way they would in a Java import statement to import all classes in the package (i.e., add .* to the full name of the package). Replace .* with .- to also analyze all subpackages.

**Analyze tests** (`sonar.findbugs.analyzeTests`): Starting with version 4.2.3 AND when running SonarQube 9.8 and above, unit tests are analyzed by default. Use this option to enable/disable the analysis of tests. See the [SonarQube documentation](https://docs.sonarqube.org/latest/project-administration/narrowing-the-focus/) for the definition of test and non-test code.

### Compiled code

FindBugs requires the compiled classes to run, if the project has JSP files they will need to be precompiled.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.io.Writer;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.LinkedList;
import java.util.List;
Expand All @@ -42,6 +43,7 @@
import org.apache.commons.lang.StringUtils;
import org.sonar.api.PropertyType;
import org.sonar.api.Startable;
import org.sonar.api.Plugin.Context;
import org.sonar.api.batch.ScannerSide;
import org.sonar.api.batch.fs.FilePredicates;
import org.sonar.api.batch.fs.FileSystem;
Expand All @@ -52,6 +54,7 @@
import org.sonar.api.config.PropertyDefinition;
import org.sonar.api.resources.Qualifiers;
import org.sonar.api.scan.filesystem.PathResolver;
import org.sonar.api.utils.Version;
import org.sonar.api.utils.log.Logger;
import org.sonar.api.utils.log.Loggers;
import org.sonar.plugins.findbugs.classpath.ClasspathLocator;
Expand All @@ -65,7 +68,6 @@
import org.sonar.plugins.java.Java;
import org.sonar.plugins.java.api.JavaResourceLocator;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.thoughtworks.xstream.XStream;

Expand Down Expand Up @@ -108,9 +110,11 @@ void initializeFindbugsProject(Project findbugsProject, ClasspathLocator classpa
findbugsProject.addAuxClasspathEntry(file.getCanonicalPath());
}

for (File file : classpathLocator.testClasspath()) {
//Auxiliary tests dependencies
findbugsProject.addAuxClasspathEntry(file.getCanonicalPath());
if (isAnalyzeTests()) {
for (File file : classpathLocator.testClasspath()) {
//Auxiliary tests dependencies
findbugsProject.addAuxClasspathEntry(file.getCanonicalPath());
}
}

ClassScreener classScreener = getOnlyAnalyzeFilter();
Expand Down Expand Up @@ -271,7 +275,9 @@ private List<File> buildClassFilesToAnalyze(ClasspathLocator classpathLocator) t
checkForMissingPrecompiledJsp(classFilesToAnalyze);
}

addClassFilesFromClasspath(classFilesToAnalyze, classpathLocator.testBinaryDirs());
if (isAnalyzeTests()) {
addClassFilesFromClasspath(classFilesToAnalyze, classpathLocator.testBinaryDirs());
}

return classFilesToAnalyze;
}
Expand Down Expand Up @@ -435,6 +441,10 @@ public boolean isAllowUncompiledCode() {
return config.getBoolean(FindbugsConstants.ALLOW_UNCOMPILED_CODE).orElse(FindbugsConstants.ALLOW_UNCOMPILED_CODE_VALUE);
}

public boolean isAnalyzeTests() {
return config.getBoolean(FindbugsConstants.ANALYZE_TESTS).orElse(FindbugsConstants.ANALYZE_TESTS_VALUE);
}

private File jsr305Lib;
private File annotationsLib;
private File fbContrib;
Expand Down Expand Up @@ -501,9 +511,9 @@ public File getFindSecBugsJar() {
return findSecBugs;
}

public static List<PropertyDefinition> getPropertyDefinitions() {
public static List<PropertyDefinition> getPropertyDefinitions(Context context) {
String subCategory = "FindBugs";
return ImmutableList.of(
List<PropertyDefinition> properties = Arrays.asList(
PropertyDefinition.builder(FindbugsConstants.EFFORT_PROPERTY)
.defaultValue(FindbugsConstants.EFFORT_DEFAULT_VALUE)
.category(Java.KEY)
Expand Down Expand Up @@ -566,6 +576,25 @@ public static List<PropertyDefinition> getPropertyDefinitions() {
.type(PropertyType.STRING)
.build()
);

if (context.getSonarQubeVersion().isGreaterThanOrEqual(Version.create(9, 8))) {
// The sonar-java plugin API only has the methods to get the test binaries/classpath starting with SonarQube 9.8
// For clarity we hide the property in earlier versions because it would have no effect (tests are not analyzed)
properties = new ArrayList<>(properties);
properties.add(
PropertyDefinition.builder(FindbugsConstants.ANALYZE_TESTS)
.defaultValue(Boolean.toString(FindbugsConstants.ANALYZE_TESTS_VALUE))
.category(Java.KEY)
.subCategory(subCategory)
.name("Analyze tests")
.description("Look for bugs in the project test code")
.onQualifiers(Qualifiers.PROJECT)
.type(PropertyType.BOOLEAN)
.build()
);
}

return properties;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ public final class FindbugsConstants {

public static final String ONLY_ANALYZE_PROPERTY = "sonar.findbugs.onlyAnalyze";

public static final String ANALYZE_TESTS = "sonar.findbugs.analyzeTests";
public static final boolean ANALYZE_TESTS_VALUE = true;

private FindbugsConstants() {
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public static FilePredicate[] getSupportedLanguagesFilePredicate(FilePredicates

@Override
public void define(Context context) {
context.addExtensions(FindbugsConfiguration.getPropertyDefinitions());
context.addExtensions(FindbugsConfiguration.getPropertyDefinitions(context));
context.addExtensions(Arrays.asList(
FindbugsSensor.class,
FindbugsProfileExporter.class,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.io.File;
Expand All @@ -38,6 +41,7 @@
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.Mockito;
import org.sonar.api.batch.fs.FilePredicate;
Expand Down Expand Up @@ -197,7 +201,7 @@ void should_get_findSecBugs() throws IOException {
}

@Test
public void should_get_only_analyze_filter() {
void should_get_only_analyze_filter() {
// No onlyAnalyze option present
assertNull(conf.getOnlyAnalyzeFilter());
// Empty Property
Expand Down Expand Up @@ -241,9 +245,14 @@ void scanEmptyFolderForAdditionalClasses() throws IOException {
}

@ParameterizedTest
@ValueSource(booleans = {true, false})
void should_warn_of_missing_precompiled_jsp(boolean withSq98Api) throws IOException {
setupSampleProject(false, true, false, withSq98Api);
@CsvSource({
"true,true",
"true,false",
"false,true",
"false,false",
})
void should_warn_of_missing_precompiled_jsp(boolean withSq98Api, boolean analyzeTests) throws IOException {
setupSampleProject(false, true, false, withSq98Api, analyzeTests);

try (Project project = new Project()) {
conf.initializeFindbugsProject(project, classpathLocator);
Expand All @@ -261,16 +270,28 @@ void should_warn_of_missing_precompiled_jsp(boolean withSq98Api) throws IOExcept
}

@ParameterizedTest
@ValueSource(booleans = {true, false})
void should_analyze_precompiled_jsp(boolean withSq98Api) throws IOException {
setupSampleProject(true, true, false, withSq98Api);
@CsvSource({
"true,true",
"true,false",
"false,true",
"false,false",
})
void should_analyze_precompiled_jsp(boolean withSq98Api, boolean analyzeTests) throws IOException {
setupSampleProject(true, true, false, withSq98Api, analyzeTests);

try (Project project = new Project()) {
conf.initializeFindbugsProject(project, classpathLocator);

if (withSq98Api) {
if (withSq98Api && analyzeTests) {
// we should also capture the .class that are not from JSP sources and also the unit tests
assertThat(project.getFileCount()).isEqualTo(5);

verify(classpathLocator, times(1)).testClasspath();
} else if (withSq98Api) {
// we should also capture the .class that are not from JSP sources but not the unit tests
assertThat(project.getFileCount()).isEqualTo(4);

verify(classpathLocator, never()).testClasspath();
} else {
assertThat(project.getFileCount()).isEqualTo(3);
}
Expand All @@ -280,17 +301,30 @@ void should_analyze_precompiled_jsp(boolean withSq98Api) throws IOException {
}

@ParameterizedTest
@ValueSource(booleans = {true, false})
void scala_project(boolean withSq98Api) throws IOException {
setupSampleProject(false, false, true, withSq98Api);
@CsvSource({
"true,true",
"true,false",
"false,true",
"false,false",
})
void scala_project(boolean withSq98Api, boolean analyzeTests) throws IOException {
setupSampleProject(false, false, true, withSq98Api, analyzeTests);

try (Project project = new Project()) {
conf.initializeFindbugsProject(project, classpathLocator);

if (withSq98Api) {
if (withSq98Api && analyzeTests) {
assertThat(project.getFileCount()).isEqualTo(2);
assertThat(project.getFile(0)).endsWith("Test.class");
assertThat(project.getFile(1)).endsWith("UnitTest.class");

verify(classpathLocator, times(1)).testClasspath();
} else if (withSq98Api) {
assertThat(project.getFileCount()).isEqualTo(1);
// Even though it is named "Test" it is in the "main" folder so it should be analyzed
assertThat(project.getFile(0)).endsWith("Test.class");

verify(classpathLocator, never()).testClasspath();
} else {
assertThat(project.getFileCount()).isEqualTo(1);
assertThat(project.getFile(0)).endsWith("Test.class");
Expand All @@ -300,12 +334,20 @@ void scala_project(boolean withSq98Api) throws IOException {
assertThat(logTester.getLogs(LoggerLevel.WARN)).isNull();
}

private void setupSampleProject(boolean withPrecompiledJsp, boolean withJspFiles, boolean withScalaFiles, boolean withSq98Api) throws IOException {
private void setupSampleProject(boolean withPrecompiledJsp,
boolean withJspFiles,
boolean withScalaFiles,
boolean withSq98Api,
boolean analyzeTests) throws IOException {
configuration.setProperty(FindbugsConstants.ANALYZE_TESTS, Boolean.toString(analyzeTests));

mockHasLanguagePredicate(withJspFiles, "jsp");
mockHasLanguagesPredicate(withScalaFiles, "scala");

// classpath
// |_ some.jar
// |_ some-test.jar
// |_ some-other-test.jar
// |_ target
// |_ jsp_servlet
// |_ classes
Expand All @@ -318,6 +360,8 @@ private void setupSampleProject(boolean withPrecompiledJsp, boolean withJspFiles
// |_ UnitTest.class
File classpath = new File(temp, "classpath");
File jarFile = new File(classpath, "some.jar");
File testJarFile = new File(classpath, "some-test.jar");
File testOtherJarFile = new File(classpath, "some-other-test.jar");
File targetFolder = new File(classpath, "target");
File classesFolder = new File(targetFolder, "classes");
File jspServletFolder = new File(targetFolder, "jsp_servlet");
Expand Down Expand Up @@ -352,8 +396,10 @@ private void setupSampleProject(boolean withPrecompiledJsp, boolean withJspFiles
}

List<File> classpathFiles = Arrays.asList(jarFile, classesFolder, jspServletFolder, moduleInfoFile);
List<File> testClasspathFiles = Arrays.asList(testJarFile, testOtherJarFile);

when(javaResourceLocator.classpath()).thenReturn(classpathFiles);
when(classpathLocator.testClasspath()).thenReturn(testClasspathFiles);

if (withSq98Api) {
List<File> binaryDirs = Arrays.asList(classesFolder, jspServletFolder);
Expand Down
21 changes: 15 additions & 6 deletions src/test/java/org/sonar/plugins/findbugs/FindbugsPluginTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,32 @@

import org.sonar.api.Plugin;
import org.sonar.api.SonarRuntime;
import org.sonar.api.utils.Version;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;

class FindbugsPluginTest {

@Test
void testGetExtensions() {
@ParameterizedTest
@CsvSource({
"9.7,24",
// We expect one more extension (the "sonar.findbugs.analyzeTests" property) when the version is >= 9.8
"9.8,25"
})
void testGetExtensions(String version, int expectedExtensionsCount) {

//Plugin.Context ctx = new FindbugsPlugin.Context(Version.parse("1.33.7"));
Plugin.Context ctx = new Plugin.Context(mock(SonarRuntime.class));
SonarRuntime runtime = mock(SonarRuntime.class);
when(runtime.getApiVersion()).thenReturn(Version.parse(version));
Plugin.Context ctx = new Plugin.Context(runtime);

FindbugsPlugin plugin = new FindbugsPlugin();
plugin.define(ctx);

assertEquals(24, ctx.getExtensions().size(), "extension count");
assertEquals(expectedExtensionsCount, ctx.getExtensions().size(), "extensions count");
}
}