Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

Ktlint integration #110

Merged
merged 4 commits into from
Jul 31, 2018
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import com.novoda.staticanalysis.internal.CodeQualityConfigurator
import com.novoda.staticanalysis.internal.checkstyle.CheckstyleConfigurator
import com.novoda.staticanalysis.internal.detekt.DetektConfigurator
import com.novoda.staticanalysis.internal.findbugs.FindbugsConfigurator
import com.novoda.staticanalysis.internal.ktlint.KtlintConfigurator
import com.novoda.staticanalysis.internal.lint.LintConfigurator
import com.novoda.staticanalysis.internal.pmd.PmdConfigurator
import org.gradle.api.NamedDomainObjectContainer
Expand Down Expand Up @@ -40,6 +41,7 @@ class StaticAnalysisPlugin implements Plugin<Project> {
PmdConfigurator.create(project, violationsContainer, evaluateViolations),
FindbugsConfigurator.create(project, violationsContainer, evaluateViolations),
DetektConfigurator.create(project, violationsContainer, evaluateViolations),
KtlintConfigurator.create(project, violationsContainer, evaluateViolations),
LintConfigurator.create(project, violationsContainer, evaluateViolations)
]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package com.novoda.staticanalysis.internal

import com.novoda.staticanalysis.Violations
import org.gradle.api.DefaultTask
import org.gradle.api.tasks.InputFile
import org.gradle.api.tasks.TaskAction

abstract class CollectViolationsTask extends DefaultTask {

@InputFile
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to make the task better cacheable. Don't think it will hurt in any way. WDYT?

private File xmlReportFile
private File htmlReportFile
private Violations violations
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package com.novoda.staticanalysis.internal.ktlint

import com.novoda.staticanalysis.StaticAnalysisExtension
import com.novoda.staticanalysis.Violations
import com.novoda.staticanalysis.internal.Configurator
import com.novoda.staticanalysis.internal.VariantFilter
import com.novoda.staticanalysis.internal.checkstyle.CollectCheckstyleViolationsTask
import org.gradle.api.GradleException
import org.gradle.api.NamedDomainObjectContainer
import org.gradle.api.Project
import org.gradle.api.Task

class KtlintConfigurator implements Configurator {

private static final String KTLINT_PLUGIN = 'org.jlleitschuh.gradle.ktlint'
private static final String KTLINT_NOT_APPLIED = 'The Ktlint plugin is configured but not applied. Please apply the plugin in your build script.\nFor more information see https://github.com/jeremymailen/kotlinter-gradle'

private final Project project
private final Violations violations
private final Task evaluateViolations
private final VariantFilter variantFilter

static KtlintConfigurator create(Project project,
NamedDomainObjectContainer<Violations> violationsContainer,
Task evaluateViolations) {
Violations violations = violationsContainer.maybeCreate('ktlint')
return new KtlintConfigurator(project, violations, evaluateViolations)
}

KtlintConfigurator(Project project, Violations violations, Task evaluateViolations) {
this.project = project
this.violations = violations
this.evaluateViolations = evaluateViolations
this.variantFilter = new VariantFilter(project)
}

@Override
void execute() {
project.extensions.findByType(StaticAnalysisExtension).ext.ktlint = { Closure config ->
if (!project.plugins.hasPlugin(KTLINT_PLUGIN)) {
throw new GradleException(KTLINT_NOT_APPLIED)
}

def ktlint = project.ktlint
ktlint.ignoreFailures = true
ktlint.reporters = ['CHECKSTYLE', 'PLAIN']
ktlint.ext.includeVariants = { Closure<Boolean> filter ->
variantFilter.includeVariantsFilter = filter
}
config.delegate = ktlint
config()

project.afterEvaluate {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a notice: Had to add project.afterEvaluate since the plugin itself uses afterEvaluate too. Don't think it is a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be ok, at least for now, since as you say that's what the plugin we're wrapping does


project.plugins.withId("kotlin") {
configureKotlinProject()
}
project.plugins.withId("kotlin2js") {
configureKotlinProject()
}
project.plugins.withId("kotlin-platform-common") {
configureKotlinProject()
}
project.plugins.withId('com.android.application') {
configureAndroidWithVariants(variantFilter.filteredApplicationVariants)
}
project.plugins.withId('com.android.library') {
configureAndroidWithVariants(variantFilter.filteredLibraryVariants)
}
}
}
}

private void configureKotlinProject() {
project.sourceSets.each { configureKtlint(it) }
}

private void configureAndroidWithVariants(def mainVariants) {
mainVariants.all { configureKtlint(it) }
variantFilter.filteredTestVariants.all { configureKtlint(it) }
variantFilter.filteredUnitTestVariants.all { configureKtlint(it) }
}

private void configureKtlint(def sourceSet) {
def collectViolations = createCollectViolationsTask(violations, sourceSet.name)
evaluateViolations.dependsOn collectViolations
}

private def createCollectViolationsTask(Violations violations, def sourceSetName) {
project.tasks.create("collectKtlint${sourceSetName.capitalize()}Violations", CollectCheckstyleViolationsTask) { task ->
task.xmlReportFile = new File(project.buildDir, "reports/ktlint/ktlint-${sourceSetName}.xml")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No way to change the output in the plugin. Hardcoded for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we get ktlint-main.xml? Never used the underlying Ktlint plugin, so not sure :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Gradle plugin provides the file name and path internally to ktlint tool.

It has report per sourceSet in pure Kotlin, and report per variant in Android projects.

The path and file names are hardcoded and not configurable by the user.

task.htmlReportFile = new File(project.buildDir, "reports/ktlint/ktlint-${sourceSetName}.txt")
task.violations = violations
task.dependsOn project.tasks["ktlint${sourceSetName.capitalize()}Check"]
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
data class ValidClass(val prop1: String, val prop2: Int)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class KtLintViolator {

override fun equals(other: Any?): Boolean {
// this is not allowed
Copy link
Contributor

Choose a reason for hiding this comment

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

What is not allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha I think always returning true is not allowed. This is just a fixture and a copy paste from detekt fixtures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, I need any Kotlin code with style problem. Would be happy to change. 馃憤

return true;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
package com.novoda.staticanalysis.internal.ktlint

import com.novoda.test.Fixtures
import com.novoda.test.TestProject
import com.novoda.test.TestProjectRule
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.Parameterized

import static com.novoda.test.LogsSubject.assertThat

@RunWith(Parameterized.class)
class KtlintIntegrationTest {

private static final String KTLINT_NOT_APPLIED = 'The Ktlint plugin is configured but not applied. Please apply the plugin in your build script.'
public static final String DEFAULT_CONFIG = '''
ktlint {
includeVariants { it.name == "debug" }
}
'''
public static final String EMPTY_CONFIG = 'ktlint {}'

@Parameterized.Parameters(name = '{0}')
static def rules() {
return [
[TestProjectRule.forKotlinProject(), 'main'].toArray(),
[TestProjectRule.forAndroidKotlinProject(), 'debug'].toArray()
]
}

@Rule
public final TestProjectRule projectRule
private final String sourceSetName;

KtlintIntegrationTest(TestProjectRule projectRule, String sourceSetName) {
this.projectRule = projectRule
this.sourceSetName = sourceSetName
}

@Test
void shouldNotFailWhenKtlintIsNotConfigured() {
def result = createProjectWith(Fixtures.Ktlint.SOURCES_WITH_ERROR)
.build('evaluateViolations')

assertThat(result.logs).doesNotContainKtlintViolations()
}

@Test
void shouldFailBuildOnConfigurationWhenKtlintConfiguredButNotApplied() {
def result = projectRule.newProject()
.withToolsConfig(EMPTY_CONFIG)
.buildAndFail('evaluateViolations')

assertThat(result.logs).contains(KTLINT_NOT_APPLIED)
}

@Test
void shouldFailBuildWhenKtlintErrorsOverTheThreshold() {
def result = createProjectWith(Fixtures.Ktlint.SOURCES_WITH_ERROR)
.withToolsConfig(DEFAULT_CONFIG)
.buildAndFail('evaluateViolations')

assertThat(result.logs).containsLimitExceeded(1, 0)
assertThat(result.logs).containsKtlintViolations(1,
result.buildFileUrl("reports/ktlint/ktlint-${sourceSetName}.txt"))
}

@Test
void shouldNotFailWhenErrorsAreWithinThreshold() {
def result = createProjectWith(Fixtures.Ktlint.SOURCES_WITH_ERROR, 1)
.withToolsConfig(DEFAULT_CONFIG)
.build('evaluateViolations')

assertThat(result.logs).containsKtlintViolations(1,
result.buildFileUrl("reports/ktlint/ktlint-${sourceSetName}.txt"))
}

@Test
void shouldNotFailBuildWhenNoErrorsEncounteredAndNoThresholdTrespassed() {
def result = createProjectWith(Fixtures.Ktlint.SOURCES_NO_ERROR, 0)
.withToolsConfig(EMPTY_CONFIG)
.build('evaluateViolations')

assertThat(result.logs).doesNotContainLimitExceeded()
assertThat(result.logs).doesNotContainKtlintViolations()
}

private TestProject createProjectWith(File sources, int maxErrors = 0) {
projectRule.newProject()
.withPlugin('org.jlleitschuh.gradle.ktlint', '4.1.0')
.withSourceSet('main', sources)
.withPenalty("""{
maxWarnings = 0
maxErrors = ${maxErrors}
}""")
}
}
5 changes: 5 additions & 0 deletions plugin/src/test/groovy/com/novoda/test/Fixtures.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ public final class Fixtures {
public static final File RULES = new File(RULES_DIR, 'detekt/detekt.yml')
}

final static class Ktlint {
static final File SOURCES_WITH_ERROR = new File(SOURCES_DIR, 'ktlint/with-error')
static final File SOURCES_NO_ERROR = new File(SOURCES_DIR, 'ktlint/no-error')
}

final static class Lint {
public static final File SOURCES_WITH_WARNINGS = new File(SOURCES_DIR, 'lint/warnings')
public static final File SOURCES_WITH_ERRORS = new File(SOURCES_DIR, 'lint/errors')
Expand Down
9 changes: 9 additions & 0 deletions plugin/src/test/groovy/com/novoda/test/LogsSubject.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class LogsSubject extends Subject<LogsSubject, Logs> {
private static final String PMD_VIOLATIONS_FOUND = 'PMD violations found'
private static final String FINDBUGS_VIOLATIONS_FOUND = 'Findbugs violations found'
private static final String DETEKT_VIOLATIONS_FOUND = 'Detekt violations found'
private static final String KTLINT_VIOLATIONS_FOUND = 'ktlint violations found'
private static final String LINT_VIOLATIONS_FOUND = 'Lint violations found'

private static final SubjectFactory<LogsSubject, Logs> FACTORY = new SubjectFactory<LogsSubject, Logs>() {
Expand Down Expand Up @@ -71,6 +72,10 @@ class LogsSubject extends Subject<LogsSubject, Logs> {
outputSubject.doesNotContain(DETEKT_VIOLATIONS_FOUND)
}

public void doesNotContainKtlintViolations() {
outputSubject.doesNotContain(KTLINT_VIOLATIONS_FOUND)
}

public void doesNotContainLintViolations() {
outputSubject.doesNotContain(LINT_VIOLATIONS_FOUND)
}
Expand All @@ -91,6 +96,10 @@ class LogsSubject extends Subject<LogsSubject, Logs> {
containsToolViolations(DETEKT_VIOLATIONS_FOUND, errors, warnings, reportUrls)
}

public void containsKtlintViolations(int errors, String... reportUrls) {
containsToolViolations(KTLINT_VIOLATIONS_FOUND, errors, 0, reportUrls)
}

public void containsLintViolations(int errors, int warnings, String... reportUrls) {
containsToolViolations(LINT_VIOLATIONS_FOUND, errors, warnings, reportUrls)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ buildscript {
}
}
plugins {
${formatPlugins(project)}
${formatPlugins(project)}
id 'com.novoda.static-analysis'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is better if the static-analysis plugin is added later. I needed to do this earlier. It may not be needed anymore. But I think it is a good change anyways since it makes it explicit. Before it was added by the method withPlugin in the constructor.

}
repositories {
google()
Expand Down Expand Up @@ -58,7 +59,7 @@ ${formatExtension(project)}
.collect { Map.Entry<String, List<String>> entry ->
"""$entry.key {
manifest.srcFile '${Fixtures.ANDROID_MANIFEST}'
kotlin {
java {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Android projects uses java sourceSet for mixed Kotlin and Java sources.

${entry.value.collect { "srcDir '$it'" }.join('\n\t\t\t\t')}
}
}"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ buildscript {
}
plugins {
${formatPlugins(project)}
id 'com.novoda.static-analysis'
}
repositories {
google()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ final class TestJavaProject extends TestProject<TestJavaProject> {
private static final Closure<String> TEMPLATE = { TestProject project ->
"""
plugins {
${formatPlugins(project)}
${formatPlugins(project)}
id 'com.novoda.static-analysis'
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explained here already. Can check if reverting is fine or not. Users are expected to apply static analysis at the last position. Also makes it more obvious here.
#110 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I actually read it, but somehow missed the connection 馃檲

}
repositories {
jcenter()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,15 @@ buildscript {

plugins {
${formatPlugins(project)}
id 'com.novoda.static-analysis'
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above.

}

apply plugin: 'kotlin'

repositories {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was needed for ktlint jar to be resolved in pure kotlin project.

jcenter()
}

sourceSets {
${formatSourceSets(project)}
}
Expand Down
3 changes: 1 addition & 2 deletions plugin/src/test/groovy/com/novoda/test/TestProject.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ ${project.additionalConfiguration}
.withPluginClasspath()
.forwardStdOutput(new OutputStreamWriter(System.out))
.forwardStdError(new OutputStreamWriter(System.out))
withPlugin('com.novoda.static-analysis')
}

private static File createProjectDir(String path) {
Expand Down Expand Up @@ -126,7 +125,7 @@ ${project.additionalConfiguration}
}

protected static String formatPlugins(TestProject project) {
"${project.plugins.join('\n')}"
project.plugins.join('\n')
}

public static class Result {
Expand Down