From af85b0f6316ba31b2ef070441f2974ccb52e6fda Mon Sep 17 00:00:00 2001 From: Antonio Bertucci Date: Fri, 11 Nov 2016 14:57:16 +0100 Subject: [PATCH 1/6] Move exclude filters setup to base configurator class --- .../staticanalysis/internal/CodeQualityConfigurator.groovy | 5 ++++- .../internal/checkstyle/CheckstyleConfigurator.groovy | 3 +-- .../internal/findbugs/FindbugsConfigurator.groovy | 2 +- .../staticanalysis/internal/pmd/PmdConfigurator.groovy | 3 +-- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy b/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy index d91e157..6806890 100644 --- a/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy +++ b/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy @@ -60,6 +60,9 @@ abstract class CodeQualityConfigurator getTaskClass() - protected abstract void configureTask(T task) + protected void configureTask(T task) { + task.group = 'verification' + task.exclude(excludes) + } } diff --git a/plugin/src/main/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleConfigurator.groovy b/plugin/src/main/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleConfigurator.groovy index 52dbda4..b4099c0 100644 --- a/plugin/src/main/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleConfigurator.groovy +++ b/plugin/src/main/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleConfigurator.groovy @@ -65,11 +65,10 @@ class CheckstyleConfigurator extends CodeQualityConfigurator { @Override protected void configureTask(Pmd pmd) { - pmd.group = 'verification' + super.configureTask(pmd) pmd.ignoreFailures = true pmd.metaClass.getLogger = { QuietLogger.INSTANCE } - pmd.exclude(excludes) pmd.doLast { File xmlReportFile = pmd.reports.xml.destination File htmlReportFile = new File(xmlReportFile.absolutePath - '.xml' + '.html') From 15c158a3addb613edfd52b6d73cddedd90fec1ea Mon Sep 17 00:00:00 2001 From: Antonio Bertucci Date: Fri, 11 Nov 2016 16:18:37 +0100 Subject: [PATCH 2/6] Introduce SourceFilter to hold and apply exclude rules to SourceTask --- .../internal/SourceFilter.groovy | 24 +++++ .../internal/SourceFilterTest.groovy | 89 +++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 plugin/src/main/groovy/com/novoda/staticanalysis/internal/SourceFilter.groovy create mode 100644 plugin/src/test/groovy/com/novoda/staticanalysis/internal/SourceFilterTest.groovy diff --git a/plugin/src/main/groovy/com/novoda/staticanalysis/internal/SourceFilter.groovy b/plugin/src/main/groovy/com/novoda/staticanalysis/internal/SourceFilter.groovy new file mode 100644 index 0000000..714de23 --- /dev/null +++ b/plugin/src/main/groovy/com/novoda/staticanalysis/internal/SourceFilter.groovy @@ -0,0 +1,24 @@ +package com.novoda.staticanalysis.internal + +import org.gradle.api.file.FileCollection +import org.gradle.api.tasks.SourceTask + +class SourceFilter { + + private final List excludes = [] + + void exclude(Object rule) { + excludes.add(rule) + } + + void applyTo(SourceTask task) { + excludes.each { filter -> + if (filter instanceof FileCollection) { + task.source = task.source.findAll { !filter.contains(it) } + } else { + task.exclude(filter as String) + } + } + } + +} diff --git a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/SourceFilterTest.groovy b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/SourceFilterTest.groovy new file mode 100644 index 0000000..789d898 --- /dev/null +++ b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/SourceFilterTest.groovy @@ -0,0 +1,89 @@ +package com.novoda.staticanalysis.internal + +import org.gradle.api.Project +import org.gradle.api.tasks.SourceTask +import org.gradle.testfixtures.ProjectBuilder +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder + +import static com.google.common.truth.Truth.assertThat +import static com.novoda.test.Fixtures.Checkstyle.SOURCES_WITH_ERRORS +import static com.novoda.test.Fixtures.Checkstyle.SOURCES_WITH_WARNINGS + +class SourceFilterTest { + + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder() + + private Project project + private SourceFilter filter + + @Before + public void setUp() { + project = ProjectBuilder.builder() + .withProjectDir(temporaryFolder.newFolder()) + .build() + filter = new SourceFilter() + } + + @Test + public void shouldKeepAllSourcesWhenNoExcludeFilterProvided() { + SourceTask task = givenTaskWith(errorsSources + warningsSources) + + filter.applyTo(task) + + assertThat(task.source).containsExactlyElementsIn(errorsSources + warningsSources) + } + + @Test + public void shouldRemoveFilesMatchingThePatternWhenExcludePatternProvided() { + SourceTask task = givenTaskWith(errorsSources + warningsSources) + filter.exclude('**/*.java') + + filter.applyTo(task) + + assertThat(task.source).isEmpty() + } + + @Test + public void shouldRemoveFilesInSpecifiedFileCollectionWhenExcludeFileCollectionProvided() { + SourceTask task = givenTaskWith(errorsSources + warningsSources) + filter.exclude(project.fileTree(SOURCES_WITH_ERRORS)) + + filter.applyTo(task) + + assertThat(task.source).containsExactlyElementsIn(warningsSources) + } + + @Test + public void shouldRemoveFilesInSpecifiedFileCollectionWhenExcludeSourceSetProvided() { + project.apply plugin: 'java' + project.sourceSets.main { + java { + srcDir project.file(SOURCES_WITH_ERRORS) + } + } + SourceTask task = givenTaskWith(project.sourceSets.main.java.srcDirs) + filter.exclude(project.fileTree(SOURCES_WITH_ERRORS)) + + filter.applyTo(task) + + assertThat(task.source).isEmpty() + } + + private SourceTask givenTaskWith(Iterable files) { + project.tasks.create('someSourceTask', SourceTask) { + source = project.files(files) + } + } + + private Set getErrorsSources() { + project.fileTree(SOURCES_WITH_ERRORS).files + } + + private Set getWarningsSources() { + project.fileTree(SOURCES_WITH_WARNINGS).files + } +} From 0899aeab44e274160f4282dcccf2e1ce0272b0f4 Mon Sep 17 00:00:00 2001 From: Antonio Bertucci Date: Fri, 11 Nov 2016 16:56:31 +0100 Subject: [PATCH 3/6] Use SourceFilter in all configurators --- .../internal/CodeQualityConfigurator.groovy | 6 +++--- .../CheckstyleIntegrationTest.groovy | 21 +++++++++++++++++-- .../findbugs/FindbugsIntegrationTest.groovy | 19 ++++++++++++++++- .../internal/pmd/PmdIntegrationTest.groovy | 20 +++++++++++++++++- 4 files changed, 59 insertions(+), 7 deletions(-) diff --git a/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy b/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy index 6806890..0e35007 100644 --- a/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy +++ b/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy @@ -12,7 +12,7 @@ abstract class CodeQualityConfigurator excludes = [] + protected final SourceFilter filter = new SourceFilter() protected CodeQualityConfigurator(Project project, Violations violations, EvaluateViolationsTask evaluateViolations) { this.project = project @@ -25,7 +25,7 @@ abstract class CodeQualityConfigurator excludes.add(pattern) } + ext.exclude = { Object rule -> filter.exclude(rule) } config.delegate = it config() } @@ -62,7 +62,7 @@ abstract class CodeQualityConfigurator Date: Fri, 11 Nov 2016 18:41:00 +0100 Subject: [PATCH 4/6] Add support for single File as well as Iterable as exclude rules --- .../internal/CodeQualityConfigurator.groovy | 3 +- .../internal/SourceFilter.groovy | 31 +++++++++++++++---- .../internal/SourceFilterTest.groovy | 22 +++++++++++-- 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy b/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy index 0e35007..c9884d8 100644 --- a/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy +++ b/plugin/src/main/groovy/com/novoda/staticanalysis/internal/CodeQualityConfigurator.groovy @@ -12,12 +12,13 @@ abstract class CodeQualityConfigurator excludes = [] - void exclude(Object rule) { - excludes.add(rule) + SourceFilter(Project project) { + this.project = project + } + + void exclude(Object exclude) { + excludes.add(exclude) } void applyTo(SourceTask task) { - excludes.each { filter -> - if (filter instanceof FileCollection) { - task.source = task.source.findAll { !filter.contains(it) } + excludes.each { exclude -> + if (exclude instanceof File) { + apply(task, project.files(exclude)) + } else if (exclude instanceof FileCollection) { + apply(task, exclude) + } else if (exclude instanceof Iterable) { + apply(task, exclude.inject(null, accumulateIntoTree()) as FileTree) } else { - task.exclude(filter as String) + task.exclude(exclude as String) } } } + private void apply(SourceTask task, FileCollection excludedFiles) { + task.source = task.source.findAll { !excludedFiles.contains(it) } + } + + private def accumulateIntoTree() { + return { tree, file -> tree?.plus(project.fileTree(file)) ?: project.fileTree(file) } + } + } diff --git a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/SourceFilterTest.groovy b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/SourceFilterTest.groovy index 789d898..f7ee41e 100644 --- a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/SourceFilterTest.groovy +++ b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/SourceFilterTest.groovy @@ -25,7 +25,7 @@ class SourceFilterTest { project = ProjectBuilder.builder() .withProjectDir(temporaryFolder.newFolder()) .build() - filter = new SourceFilter() + filter = new SourceFilter(project) } @Test @@ -65,8 +65,24 @@ class SourceFilterTest { srcDir project.file(SOURCES_WITH_ERRORS) } } - SourceTask task = givenTaskWith(project.sourceSets.main.java.srcDirs) - filter.exclude(project.fileTree(SOURCES_WITH_ERRORS)) + SourceTask task = givenTaskWith(errorsSources) + filter.exclude(project.sourceSets.main.java.srcDirs) + + filter.applyTo(task) + + assertThat(task.source).isEmpty() + } + + @Test + public void shouldRemoveFilesInSpecifiedFileCollectionWhenSourceFileExcluded() { + project.apply plugin: 'java' + project.sourceSets.main { + java { + srcDir project.file(SOURCES_WITH_ERRORS) + } + } + SourceTask task = givenTaskWith(errorsSources) + filter.exclude(new File(SOURCES_WITH_ERRORS, 'Greeter.java')) filter.applyTo(task) From 01b2681ae6e24afe7e2a50ef9e1266521458e898 Mon Sep 17 00:00:00 2001 From: Antonio Bertucci Date: Fri, 11 Nov 2016 18:51:35 +0100 Subject: [PATCH 5/6] Rename tests to avoid confusion --- .../internal/checkstyle/CheckstyleIntegrationTest.groovy | 2 +- .../internal/findbugs/FindbugsIntegrationTest.groovy | 2 +- .../staticanalysis/internal/pmd/PmdIntegrationTest.groovy | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleIntegrationTest.groovy b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleIntegrationTest.groovy index 0d912cc..111450e 100644 --- a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleIntegrationTest.groovy +++ b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleIntegrationTest.groovy @@ -126,7 +126,7 @@ public class CheckstyleIntegrationTest { } @Test - public void shouldNotFailBuildWhenCheckstyleConfiguredToIgnoreFaultySourceSet() { + public void shouldNotFailBuildWhenCheckstyleConfiguredToIgnoreFaultySourceFolder() { TestProject.Result result = projectRule.newProject() .withSourceSet('main', Fixtures.Checkstyle.SOURCES_WITH_WARNINGS) .withSourceSet('test', Fixtures.Checkstyle.SOURCES_WITH_ERRORS) diff --git a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/findbugs/FindbugsIntegrationTest.groovy b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/findbugs/FindbugsIntegrationTest.groovy index 93ffdad..d36bb3e 100644 --- a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/findbugs/FindbugsIntegrationTest.groovy +++ b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/findbugs/FindbugsIntegrationTest.groovy @@ -149,7 +149,7 @@ class FindbugsIntegrationTest { } @Test - public void shouldNotFailBuildWhenFindbugsConfiguredToIgnoreFaultySourceSet() { + public void shouldNotFailBuildWhenFindbugsConfiguredToIgnoreFaultySourceFolder() { TestProject.Result result = projectRule.newProject() .withSourceSet('debug', SOURCES_WITH_LOW_VIOLATION, SOURCES_WITH_MEDIUM_VIOLATION) .withSourceSet('release', SOURCES_WITH_HIGH_VIOLATION) diff --git a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/pmd/PmdIntegrationTest.groovy b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/pmd/PmdIntegrationTest.groovy index 3ca3444..2790d46 100644 --- a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/pmd/PmdIntegrationTest.groovy +++ b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/pmd/PmdIntegrationTest.groovy @@ -146,7 +146,7 @@ public class PmdIntegrationTest { } @Test - public void shouldNotFailBuildWhenPmdConfiguredToIgnoreFaultySourceSets() { + public void shouldNotFailBuildWhenPmdConfiguredToIgnoreFaultySourceFolders() { TestProject.Result result = projectRule.newProject() .withSourceSet('main', Fixtures.Pmd.SOURCES_WITH_PRIORITY_1_VIOLATION) .withSourceSet('main2', Fixtures.Pmd.SOURCES_WITH_PRIORITY_2_VIOLATION) From 77be5ee38cb87c77de96ac55c2f688a5c4445634 Mon Sep 17 00:00:00 2001 From: Antonio Bertucci Date: Fri, 11 Nov 2016 19:03:26 +0100 Subject: [PATCH 6/6] Add missing tests using source sets as exclude rules --- .../CheckstyleIntegrationTest.groovy | 18 ++++++++++++++++++ .../findbugs/FindbugsIntegrationTest.groovy | 17 +++++++++++++++++ .../internal/pmd/PmdIntegrationTest.groovy | 18 ++++++++++++++++++ .../com/novoda/test/TestProjectRule.groovy | 12 +++++++++--- 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleIntegrationTest.groovy b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleIntegrationTest.groovy index 111450e..769ea5a 100644 --- a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleIntegrationTest.groovy +++ b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/checkstyle/CheckstyleIntegrationTest.groovy @@ -143,6 +143,24 @@ public class CheckstyleIntegrationTest { result.buildFile('reports/checkstyle/main.html')) } + @Test + public void shouldNotFailBuildWhenCheckstyleConfiguredToIgnoreFaultySourceSet() { + TestProject.Result result = projectRule.newProject() + .withSourceSet('main', Fixtures.Checkstyle.SOURCES_WITH_WARNINGS) + .withSourceSet('test', Fixtures.Checkstyle.SOURCES_WITH_ERRORS) + .withFile(Fixtures.Checkstyle.MODULES, 'config/checkstyle/checkstyle.xml') + .withPenalty('''{ + maxWarnings = 1 + maxErrors = 0 + }''') + .withCheckstyle(checkstyle(DEFAULT_CONFIG, "exclude ${projectRule.printSourceSet('test')}.java.srcDirs")) + .build('check') + + assertThat(result.logs).doesNotContainLimitExceeded() + assertThat(result.logs).containsCheckstyleViolations(0, 1, + result.buildFile('reports/checkstyle/main.html')) + } + @Test public void shouldNotFailWhenCheckstyleNotConfigured() { TestProject.Result result = projectRule.newProject() diff --git a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/findbugs/FindbugsIntegrationTest.groovy b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/findbugs/FindbugsIntegrationTest.groovy index d36bb3e..6f2979a 100644 --- a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/findbugs/FindbugsIntegrationTest.groovy +++ b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/findbugs/FindbugsIntegrationTest.groovy @@ -165,6 +165,23 @@ class FindbugsIntegrationTest { result.buildFile('reports/findbugs/debug.html')) } + @Test + public void shouldNotFailBuildWhenFindbugsConfiguredToIgnoreFaultySourceSet() { + TestProject.Result result = projectRule.newProject() + .withSourceSet('debug', SOURCES_WITH_LOW_VIOLATION, SOURCES_WITH_MEDIUM_VIOLATION) + .withSourceSet('release', SOURCES_WITH_HIGH_VIOLATION) + .withPenalty('''{ + maxErrors = 0 + maxWarnings = 10 + }''') + .withFindbugs("findbugs { exclude ${projectRule.printSourceSet('release')}.java.srcDirs }") + .build('check') + + assertThat(result.logs).doesNotContainLimitExceeded() + assertThat(result.logs).containsFindbugsViolations(0, 2, + result.buildFile('reports/findbugs/debug.html')) + } + @Test public void shouldCollectDuplicatedFindbugsWarningsAndErrorsAcrossAndroidVariantsForSharedSourceSets() { TestProject project = projectRule.newProject() diff --git a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/pmd/PmdIntegrationTest.groovy b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/pmd/PmdIntegrationTest.groovy index 2790d46..6758848 100644 --- a/plugin/src/test/groovy/com/novoda/staticanalysis/internal/pmd/PmdIntegrationTest.groovy +++ b/plugin/src/test/groovy/com/novoda/staticanalysis/internal/pmd/PmdIntegrationTest.groovy @@ -163,6 +163,24 @@ public class PmdIntegrationTest { assertThat(result.logs).doesNotContainPmdViolations() } + @Test + public void shouldNotFailBuildWhenPmdConfiguredToIgnoreFaultySourceSets() { + TestProject.Result result = projectRule.newProject() + .withSourceSet('main', Fixtures.Pmd.SOURCES_WITH_PRIORITY_1_VIOLATION) + .withSourceSet('main2', Fixtures.Pmd.SOURCES_WITH_PRIORITY_2_VIOLATION) + .withPenalty('''{ + maxWarnings = 0 + maxErrors = 0 + }''') + .withPmd(pmd("project.files('${Fixtures.Pmd.RULES.path}')", + "exclude ${projectRule.printSourceSet('main')}.java.srcDirs", + "exclude ${projectRule.printSourceSet('main2')}.java.srcDirs")) + .build('check') + + assertThat(result.logs).doesNotContainLimitExceeded() + assertThat(result.logs).doesNotContainPmdViolations() + } + @Test public void shouldNotFailBuildWhenPmdNotConfigured() { TestProject.Result result = projectRule.newProject() diff --git a/plugin/src/test/groovy/com/novoda/test/TestProjectRule.groovy b/plugin/src/test/groovy/com/novoda/test/TestProjectRule.groovy index 460abc9..b8f7a4e 100644 --- a/plugin/src/test/groovy/com/novoda/test/TestProjectRule.groovy +++ b/plugin/src/test/groovy/com/novoda/test/TestProjectRule.groovy @@ -7,18 +7,20 @@ import org.junit.runners.model.Statement final class TestProjectRule implements TestRule { private final Closure projectFactory + private final Closure sourceSetNameFactory private TestProject project static TestProjectRule forJavaProject() { - new TestProjectRule({ new TestJavaProject() }) + new TestProjectRule({ new TestJavaProject() }, { String name -> "project.sourceSets.$name" }) } static TestProjectRule forAndroidProject() { - new TestProjectRule({ new TestAndroidProject() }) + new TestProjectRule({ new TestAndroidProject() }, { String name -> "project.android.sourceSets.$name" }) } - private TestProjectRule(Closure projectFactory) { + private TestProjectRule(Closure projectFactory, Closure sourceSetNameFactory) { this.projectFactory = projectFactory + this.sourceSetNameFactory = sourceSetNameFactory } public TestProject newProject() { @@ -26,6 +28,10 @@ final class TestProjectRule implements TestRule { return project } + public String printSourceSet(String name) { + sourceSetNameFactory.call(name) + } + @Override Statement apply(Statement base, Description description) { return new Statement() {