Android lint variant filter support #79
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package com.novoda.staticanalysis.internal | ||
|
|
||
| import org.gradle.api.DomainObjectSet | ||
| import org.gradle.api.NamedDomainObjectSet | ||
|
|
||
| trait VariantAware { | ||
|
|
||
| Closure<Boolean> includeVariantsFilter | ||
|
|
||
| final variantSelector = { variants -> | ||
|
There was a problem hiding this comment. can't this just be a normal private method then? it's weird to see it defined it like this... There was a problem hiding this comment. Method didn't work how I wanted it to be with this trait thingy. I guess I can do Tobi's suggestion and make this a real collaborator class. |
||
| includeVariantsFilter != null ? variants.matching { includeVariantsFilter(it) } : variants | ||
| } | ||
|
|
||
| DomainObjectSet<Object> getFilteredApplicationVariants() { | ||
| variantSelector(project.android.applicationVariants) | ||
| } | ||
|
|
||
| NamedDomainObjectSet<Object> getFilteredApplicationAndTestVariants() { | ||
| variantSelector(getAllVariants(project.android.applicationVariants)) | ||
| } | ||
|
|
||
| DomainObjectSet<Object> getFilteredLibraryVariants() { | ||
| variantSelector(project.android.libraryVariants) | ||
| } | ||
|
|
||
| NamedDomainObjectSet<Object> getFilteredLibraryAndTestVariants() { | ||
| variantSelector(getAllVariants(project.android.libraryVariants)) | ||
| } | ||
|
|
||
| private NamedDomainObjectSet<Object> getAllVariants(variants1) { | ||
| NamedDomainObjectSet<Object> variants = project.container(Object) | ||
| variants.addAll(variants1) | ||
| variants.addAll(project.android.testVariants) | ||
| variants.addAll(project.android.unitTestVariants) | ||
| return variants | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,13 @@ package com.novoda.staticanalysis.internal.lint | |
| import com.novoda.staticanalysis.StaticAnalysisExtension | ||
| import com.novoda.staticanalysis.Violations | ||
| import com.novoda.staticanalysis.internal.Configurator | ||
| import com.novoda.staticanalysis.internal.VariantAware | ||
| import org.gradle.api.DomainObjectSet | ||
| import org.gradle.api.NamedDomainObjectContainer | ||
| import org.gradle.api.Project | ||
| import org.gradle.api.Task | ||
|
|
||
| class LintConfigurator implements Configurator, VariantAware { | ||
|
|
||
| private final Project project | ||
| private final Violations violations | ||
|
|
@@ -28,16 +30,30 @@ class LintConfigurator implements Configurator { | |
|
|
||
| @Override | ||
| void execute() { | ||
| project.extensions.findByType(StaticAnalysisExtension).ext.lintOptions = { Closure config -> | ||
| project.plugins.withId('com.android.application') { | ||
| configureWithVariants(config, filteredApplicationVariants) | ||
| } | ||
| project.plugins.withId('com.android.library') { | ||
| configureWithVariants(config, filteredLibraryVariants) | ||
| } | ||
|
|
||
|
|
||
| } | ||
| } | ||
|
|
||
| private void configureWithVariants(Closure config, DomainObjectSet variants) { | ||
| configureLint(config) | ||
| if (includeVariantsFilter != null) { | ||
| variants.all { configureCollectViolationsTask(it) } | ||
| } else { | ||
| configureCollectViolationsTask() | ||
| } | ||
| } | ||
|
|
||
| private void configureLint(Closure config) { | ||
| project.android.lintOptions.ext.includeVariants = { Closure<Boolean> filter -> | ||
|
There was a problem hiding this comment. @mr-archano There was a problem hiding this comment. I'm not sure what you're asking @tobiasheine (as I didn't write this), but I will try to explain what I can see going on here: L54 adds an extension method to There was a problem hiding this comment. Right, he was asking because this is a copy paste from the other ones. There was a problem hiding this comment. I understand that we define a extension |
||
| includeVariantsFilter = filter | ||
| } | ||
| project.android.lintOptions(config) | ||
| project.android.lintOptions { | ||
| xmlReport = true | ||
|
|
@@ -46,32 +62,29 @@ class LintConfigurator implements Configurator { | |
| } | ||
| } | ||
|
|
||
| private void configureCollectViolationsTask(variant = null) { | ||
| def taskSuffix = variant ? variant.name : '' | ||
| def collectViolations = createCollectViolationsTask(taskSuffix, violations).with { | ||
| it.dependsOn project.tasks.findByName("lint${taskSuffix.capitalize()}") | ||
| } | ||
| evaluateViolations.dependsOn collectViolations | ||
| } | ||
|
|
||
| private CollectLintViolationsTask createCollectViolationsTask(String taskSuffix, Violations violations) { | ||
| project.tasks.create("collectLint${taskSuffix.capitalize()}Violations", CollectLintViolationsTask) { task -> | ||
| def reportSuffix = taskSuffix ? "-$taskSuffix" : '' | ||
| task.xmlReportFile = xmlOutputFileFor(reportSuffix) | ||
| task.htmlReportFile = new File(defaultOutputFolder, "lint-results${reportSuffix}.html") | ||
| task.violations = violations | ||
| } | ||
| } | ||
|
|
||
| private File xmlOutputFileFor(reportSuffix) { | ||
|
There was a problem hiding this comment. I would prefer to push the report name directly and take the decision up two levels from here, so to keep it close to where we call There was a problem hiding this comment. I'm not sure what you mean by 2 level. Do mean the There was a problem hiding this comment. what I mean is that instead of adding this suffix logic you can move the decision of specifying what task name and what report name to use up to L36 and L38 (in that case no need of if anymore, right?) |
||
| project.android.lintOptions.xmlOutput ?: new File(defaultOutputFolder, "lint-results${reportSuffix}.xml") | ||
|
There was a problem hiding this comment. does this mean that when the user sets There was a problem hiding this comment. No idea. ☺ Good question. Only way to find out is to try. I don't think the documentation mentions that. There was a problem hiding this comment. It overrides the files. If you have xmlOuput defined, you get a single output no matter which lint task you run. So this seems to be the right way to do. But also we should do it for HTML report actually. Same happens for that too. There was a problem hiding this comment. that's weird behaviour. I agree that this is in line with the default reporting, but I wonder if we should provide a better way to handle that when using variant-aware lint tasks (ie: log a warning/error, or ignore that, or try to generate variant-aware reports from There was a problem hiding this comment. I agree. We can talk to Android team about this. I don't think it is possible to do variant-aware custom |
||
| } | ||
|
|
||
| private File getDefaultOutputFolder() { | ||
| new File(project.buildDir, 'reports') | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| package com.novoda.staticanalysis.internal.lint | ||
|
|
||
| import com.google.common.truth.Truth | ||
| import com.novoda.test.Fixtures | ||
| import com.novoda.test.TestAndroidProject | ||
| import com.novoda.test.TestProject | ||
| import com.novoda.test.TestProjectRule | ||
| import org.junit.Rule | ||
| import org.junit.Test | ||
|
|
||
| import static com.novoda.test.LogsSubject.assertThat | ||
|
|
||
| class LintAndroidVariantIntegrationTest { | ||
|
|
||
| @Rule | ||
| public final TestProjectRule<TestAndroidProject> projectRule = TestProjectRule.forAndroidProject() | ||
|
|
||
| @Test | ||
| void shouldFailBuildWhenLintViolationsOverThreshold() { | ||
| TestProject.Result result = starterProject() | ||
| .withSourceSet('demo', Fixtures.Lint.SOURCES_WITH_ERRORS) | ||
| .withToolsConfig(DEFAULT_CONFIG) | ||
| .buildAndFail('evaluateViolations') | ||
|
|
||
| assertThat(result.logs).containsLimitExceeded(1, 1) | ||
| assertThat(result.logs).containsLintViolations(1, 1, | ||
| 'reports/lint-results.html') | ||
| } | ||
|
|
||
| @Test | ||
| void givenVariantsFilteredShouldFailBuildWithDuplicatedNumbers() { | ||
| TestProject.Result result = starterProject() | ||
| .withSourceSet('demo', Fixtures.Lint.SOURCES_WITH_ERRORS) | ||
| .withToolsConfig(configWithVariants('demoDebug', 'fullRelease')) | ||
| .buildAndFail('evaluateViolations') | ||
|
|
||
| assertThat(result.logs).containsLimitExceeded(1, 2) | ||
| assertThat(result.logs).containsLintViolations(1, 2, | ||
| 'reports/lint-results-demoDebug.html', | ||
| 'reports/lint-results-fullRelease.html' | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| void shouldIgnoreErrorsFromInactiveVariant() { | ||
| TestProject.Result result = starterProject(maxWarnings: 1) | ||
| .withSourceSet('demo', Fixtures.Lint.SOURCES_WITH_ERRORS) | ||
| .withToolsConfig(configWithVariants('fullRelease')) | ||
| .build('evaluateViolations') | ||
|
|
||
| assertThat(result.logs).containsLintViolations(0, 1) | ||
| } | ||
|
|
||
| @Test | ||
| void shouldContainCollectLintTasks() { | ||
| TestProject.Result result = starterProject(maxWarnings: 1) | ||
| .withToolsConfig(DEFAULT_CONFIG) | ||
| .build('evaluateViolations') | ||
|
|
||
| Truth.assertThat(result.tasksPaths).containsAllOf( | ||
| ':lint', | ||
| ':collectLintViolations', | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| void givenVariantsFilteredShouldContainTasksForIncludedVariantsOnly() { | ||
| TestProject.Result result = starterProject(maxWarnings: 1) | ||
| .withToolsConfig(configWithVariants('demoDebug')) | ||
| .build('evaluateViolations') | ||
|
|
||
| Truth.assertThat(result.tasksPaths).containsAllOf( | ||
| ':lintDemoDebug', | ||
| ':collectLintDemoDebugViolations') | ||
| Truth.assertThat(result.tasksPaths).containsNoneOf( | ||
| ':lint', | ||
| ':lintDemoRelease', | ||
| ':collectLintDemoReleaseViolations', | ||
| ':lintFullDebug', | ||
| ':collectLintFullDebugViolations', | ||
| ':lintFullRelease', | ||
| ':collectLintFullReleaseViolations') | ||
| } | ||
|
|
||
| private static final String DEFAULT_CONFIG = | ||
| """ | ||
| lintOptions { | ||
| lintConfig = file("${Fixtures.Lint.RULES}") | ||
| } | ||
| """ | ||
|
|
||
| private static configWithVariants(String... variantNames) { | ||
| def commaSeparatedVariants = variantNames.collect { "'$it'" }.join(', ') | ||
| """ | ||
| lintOptions { | ||
| checkReleaseBuilds false | ||
| lintConfig = file("${Fixtures.Lint.RULES}") | ||
| includeVariants { it.name in [$commaSeparatedVariants] } | ||
| } | ||
| """ | ||
| } | ||
|
|
||
| private TestAndroidProject starterProject(def args = [:]) { | ||
| projectRule.newProject() | ||
| .withSourceSet('main', Fixtures.Lint.SOURCES_WITH_WARNINGS) | ||
| .withPenalty("""{ | ||
| maxErrors = ${args.maxErrors ?: 0} | ||
| maxWarnings = ${args.maxWarnings ?: 0} | ||
| }""") | ||
| .withAdditionalAndroidConfig(''' | ||
| flavorDimensions 'tier' | ||
|
|
||
| productFlavors { | ||
| demo { dimension 'tier' } | ||
| full { dimension 'tier' } | ||
| } | ||
| ''') | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would haven chosen composition over inheritance here since we mostly expose fields and methods to be shared.