[PT-511] Configure lint via SAP extension #51

tobiasheine · 2018-01-30T16:31:25Z

PT-511

Description

Scope of this PR is to add basic support for Android Lint to the SAP plugin.

This targets a feature branch.

Considerations

Added CollectLintViolationsTask which parses lint-result.xml and appends errors and warnings to the violations
Added LintConfigurator which applies the lintConfig closure to Lint and orchestrates the tasks

Tests

Added a test for the CollectLintViolationsTask to verify we parse the lint-result.xml correctly
Added the LintIntegrationTest which verifies warnings and errors are considered for the thresholds

Paired with @lgvalle on parts of it

…o maven in this test

…d projects.

This reverts commit 6a1a943.

tobiasheine · 2018-01-30T16:32:36Z

plugin/src/test/groovy/com/novoda/staticanalysis/internal/detekt/DetektIntegrationTest.groovy

@@ -15,11 +13,6 @@ import static com.novoda.test.LogsSubject.assertThat
 @RunWith(Parameterized.class)
 class DetektIntegrationTest {

-    @ClassRule


Just some boy scouting, we're not using the artifact in this test.

tobiasheine · 2018-01-30T16:37:08Z

plugin/src/test/groovy/com/novoda/test/TestAndroidProject.groovy

@@ -63,11 +63,6 @@ ${formatExtension(project)}
        .join('\n\t\t')
    }

-    @Override


@mr-archano what was the reasons for this? Can we kill it? All tests are still passing...

the reason for this was to temporary disable lint as the default configuration breaks the build on error. I would have expected the other integration tests to fail if these lines are removed, mmm

tasomaniac

Nice. Have 1 confusion and small comments.

tasomaniac · 2018-01-30T16:35:31Z

plugin/src/test/groovy/com/novoda/test/TestAndroidProject.groovy

@@ -63,11 +63,6 @@ ${formatExtension(project)}
        .join('\n\t\t')
    }

-    @Override
-    List<String> defaultArguments() {
-        ['-x', 'lint'] + super.defaultArguments()


tasomaniac · 2018-01-30T16:36:40Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+    }
+
+    private CollectLintViolationsTask createCollectViolationsTask(Violations violations) {
+        def outputFolder = new File(project.projectDir, 'build/reports')


We shouldn't hardcode build/reports, android team may change the location. I wonder if the lint task has output?

Yep, I agree. Also the user can change the location.

tasomaniac · 2018-01-30T16:41:08Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+                return
+            }
+
+            project.extensions.findByName('android').lintOptions(config)


I'm not sure if you like more groovy magic but I believe this can be replaced by
project.android.lintOptions(config)

tasomaniac · 2018-01-30T16:41:20Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+    static LintConfigurator create(Project project,
+                                   NamedDomainObjectContainer<Violations> violationsContainer,
+                                   Task evaluateViolations) {
+        Violations violations = violationsContainer.maybeCreate('Lint')


I'm confused by this maybeCreate('Lint')
This means that the container is supposed to have a named closure called Lint but I couldn't find another usage. Not in the tests for example.

I am also confused since I don't fully understand what's going in here. Tbh I just copy pasted it from the other configurators. From what I understand we pass the same Violations to each Configurator. But I don't understand how this relates to it.

Maybe @mr-archano can enlighten us around this.

We just hold all the violations in the plugin extension to keep the output of each tool output in memory before the final evaluation (see StaticAnalysisExtension.allViolations).
Each tool is now responsible of creating the Violations entry in the container, that is then passed to the evaluateViolations task.

As already mentioned IRL couple of times this is a pretty lame way of collecting all the info, and I already logged an issue to get rid of this in the next future: #43

tobiasheine · 2018-01-30T16:43:59Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+    }
+
+    private CollectLintViolationsTask createCollectViolationsTask(Violations violations) {
+        def outputFolder = new File(project.projectDir, 'build/reports')


As follow-up it would be good to resolve this path dynamically. Otherwise this will break in case the user configures another xmlOutput.

we should fix this now, as we shouldn't hardcode anything that can be configured. The only constraint we actually have is that we need to get this information after lint is configured, in order to read the correct path to the xml report. We have two ways of doing this as I see it: we get it from lintOptions or from the lint task.
If we assume that the user will configure lint through our plugin - effectively done at L36 - then we should be able to read the value from the lint extension right afterwards, right?

Also keep in mind that lint is run for all variants. Maybe we should have includeVariants here too. But in any case, should we also support multiple outputs?

@mr-archano this PR is towards a feature branch. Not on develop. So I think we can just merge it without this? with a follow-up soon?

@mr-archano just checked that, but the output path of lintOptions is null in case the user hasn't configured it. So we need to find another way. I agree with @tasomaniac to follow up on this as part of this ticket.

We agreed on for now to check whether the output path of lintOptions is configured or to fall back to the hardcoded one.

@mr-archano done

rock3r · 2018-01-30T16:40:49Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+    @Override
+    void execute() {
+        project.extensions.findByType(StaticAnalysisExtension).ext."lintOptions" = { Closure config ->
+


Nit: extra whiteline

rock3r · 2018-01-30T16:41:53Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+
+    private CollectLintViolationsTask createCollectViolationsTask(Violations violations) {
+        def outputFolder = new File(project.projectDir, 'build/reports')
+        project.tasks.create("collectLintViolations", CollectLintViolationsTask) { collectViolations ->


Nit: use single quotes for "collectLintViolations" for consistency

rock3r · 2018-01-30T16:52:45Z

...src/test/groovy/com/novoda/staticanalysis/internal/lint/CollectLintViolationsTaskTest.groovy

+        Project project = ProjectBuilder.builder().build()
+        def task = project.task('collectLintViolationsTask', type: CollectLintViolationsTask)
+
+        Violations violations = new Violations("Android Lint")


Nit: use single quotes for "Android Lint" for consistency

rock3r · 2018-01-30T16:53:25Z

plugin/src/test/fixtures/sources/lint/warnings/Warning.java

+public class Warning {
+
+    public Warning() {
+        // assertions are ignored at runtime


Does this mean the plugin can't deal with Lint warnings, but only with errors? Not really clear to me

Nope, using assertions in code results in a Warning for lint. This is just a fixture that can be used for tests when provoking a lint warning.

Oh I'm blind and I hadn't seen the path. My bad, sorry :)

rock3r · 2018-01-30T16:54:57Z

plugin/src/test/groovy/com/novoda/staticanalysis/internal/lint/LintIntegrationTest.groovy

+        testProject.withToolsConfig(lintConfiguration())
+    }
+
+    private static GString lintConfiguration() {


Why not a constant or inlined as a string?

mr-archano · 2018-01-30T22:17:49Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+                return
+            }
+
+            project.android.lintOptions(config)


This seems to be missing the bit where we avoid lint to break the build (we want to be in charge of when doing that). I believe we should force abortOnError false here.

Having a look at the whole options here: https://google.github.io/android-gradle-dsl/current/com.android.build.gradle.internal.dsl.LintOptions.html

I think we can also add xmlReport true to make sure that xml report is enabled.

Same for the htmlReport I'd say

mr-archano · 2018-01-30T22:46:28Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+    }
+
+    private CollectLintViolationsTask createCollectViolationsTask(Violations violations) {
+        def outputFolder = new File(project.projectDir, 'build/reports')


we should fix this now, as we shouldn't hardcode anything that can be configured. The only constraint we actually have is that we need to get this information after lint is configured, in order to read the correct path to the xml report. We have two ways of doing this as I see it: we get it from lintOptions or from the lint task.
If we assume that the user will configure lint through our plugin - effectively done at L36 - then we should be able to read the value from the lint extension right afterwards, right?

mr-archano · 2018-01-30T22:54:11Z

plugin/src/test/groovy/com/novoda/test/TestAndroidProject.groovy

@@ -63,11 +63,6 @@ ${formatExtension(project)}
        .join('\n\t\t')
    }

-    @Override


the reason for this was to temporary disable lint as the default configuration breaks the build on error. I would have expected the other integration tests to fail if these lines are removed, mmm

mr-archano · 2018-01-31T10:26:42Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

            collectViolations.violations = violations
        }
    }

+    private File xmlOutputFile() {


sorry to be a PITA, but this could be a getter getXmlOutputFile(), so you can use it simply at L58 as xmlOutputFile property

mr-archano · 2018-01-31T10:27:09Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+        project.android.lintOptions.xmlOutput ?: new File(defaultOutputFolder(), 'lint-results.xml')
+    }
+
+    private File defaultOutputFolder() {


mr-archano · 2018-01-31T10:28:05Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/lint/LintConfigurator.groovy

+    }
+
+    private File defaultOutputFolder() {
+        new File(project.projectDir, 'build/reports')


you should never add build/, but rather use project.buildDir. this will become then:

new File(project.buildDir, 'reports')

obsolete review

tobiasheine added 23 commits January 30, 2018 11:28

Add warning and error fixture for android lint

e312729

Remove not needed deploy test rule since we do not deploy the rules t…

c09d411

…o maven in this test

Link lint fixtures

ac10bbf

Verify the build should fail when lint warnings over threshold

54cfacf

Remove not needed TestRule since we android lint just supports androi…

7654f46

…d projects.

Add lint sample report fixture

18ec334

Verify we collect lint violations from the lint sample report

78741dc

Configure lint via plugin extension

c8997a0

Do not exclude lint for android projects

1e6f15d

Lint expects link to file instead of path as string

a29d580

Bump build tool version to suppress unwanted lint warning

6a1a943

Resolve report directory manually

ddd8694

Apply lint test rule

dd999db

Revert "Bump build tool version to suppress unwanted lint warning"

566c447

This reverts commit 6a1a943.

Ignore GradleDependency warning to keep the test stable

895fa14

Remove not needed comment

62b3d68

Fix typo

7cf29a6

Parse lint xml result and append errors and warnings to Violations

d763d95

Add static import for Truth.assertThat

b246d87

Verify the build passes when warnings within thresholds

f4be705

Verify the html report is linked in the log

0e3ea24

Verify the build fails when errors over thresholds

50d18cc

Verify the build does not fail when erros within thresholds

da8d0ca

tobiasheine requested review from lgvalle, tasomaniac and mr-archano January 30, 2018 16:31

tobiasheine commented Jan 30, 2018

View reviewed changes

rock3r self-requested a review January 30, 2018 16:38

tasomaniac reviewed Jan 30, 2018

View reviewed changes

tobiasheine commented Jan 30, 2018

View reviewed changes

Simplify lint configuration

b69c39d

rock3r reviewed Jan 30, 2018

View reviewed changes

tobiasheine added 4 commits January 30, 2018 17:57

Remove empty line

46e2755

Use single quotes

b2f473a

Use single quoted string

57ee698

Extract constant for lint configuration

beab4dc

rock3r approved these changes Jan 30, 2018

View reviewed changes

tasomaniac approved these changes Jan 30, 2018

View reviewed changes

mr-archano suggested changes Jan 30, 2018

View reviewed changes

Force xml report and not to fail on any error

ff98b70

tobiasheine force-pushed the PT-511/configure_lint_via_extension branch from 5744c3b to ff98b70 Compare January 31, 2018 09:10

tobiasheine added 2 commits January 31, 2018 10:17

Force html report since we link it

45bc335

Use configured xml output file or fall back to hardcoded one

fef16f9

mr-archano previously requested changes Jan 31, 2018

View reviewed changes

tobiasheine added 2 commits January 31, 2018 11:30

Use a getter

b40350b

Use project.buildDir rather than building it manually

b1a9a95

mr-archano approved these changes Jan 31, 2018

View reviewed changes

mr-archano merged commit 8db73ac into PT-282/integrate_android_lint Jan 31, 2018

mr-archano deleted the PT-511/configure_lint_via_extension branch January 31, 2018 11:06

tobiasheine mentioned this pull request Feb 1, 2018

[PT-282] Integrate Android Lint #62

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PT-511] Configure lint via SAP extension #51

[PT-511] Configure lint via SAP extension #51

tobiasheine commented Jan 30, 2018 •

edited by rock3r

Loading

tobiasheine Jan 30, 2018

tobiasheine Jan 30, 2018

mr-archano Jan 30, 2018

tasomaniac left a comment

tasomaniac Jan 30, 2018

tasomaniac Jan 30, 2018

tobiasheine Jan 30, 2018

tasomaniac Jan 30, 2018

tasomaniac Jan 30, 2018

tobiasheine Jan 30, 2018

tasomaniac Jan 30, 2018

mr-archano Jan 30, 2018

tobiasheine Jan 30, 2018 •

edited

Loading

mr-archano Jan 30, 2018 •

edited

Loading

tasomaniac Jan 31, 2018

tasomaniac Jan 31, 2018

tobiasheine Jan 31, 2018

tobiasheine Jan 31, 2018

tobiasheine Jan 31, 2018

rock3r Jan 30, 2018

rock3r Jan 30, 2018

rock3r Jan 30, 2018

rock3r Jan 30, 2018

tobiasheine Jan 30, 2018

rock3r Jan 30, 2018

rock3r Jan 30, 2018

mr-archano Jan 30, 2018

tasomaniac Jan 31, 2018

tobiasheine Jan 31, 2018

mr-archano Jan 30, 2018 •

edited

Loading

mr-archano Jan 30, 2018

mr-archano Jan 31, 2018

mr-archano Jan 31, 2018

mr-archano Jan 31, 2018

[PT-511] Configure lint via SAP extension #51

[PT-511] Configure lint via SAP extension #51

Conversation

tobiasheine commented Jan 30, 2018 • edited by rock3r Loading

Description

Considerations

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tasomaniac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasheine Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

mr-archano Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-archano Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasheine commented Jan 30, 2018 •

edited by rock3r

Loading

tobiasheine Jan 30, 2018 •

edited

Loading

mr-archano Jan 30, 2018 •

edited

Loading

mr-archano Jan 30, 2018 •

edited

Loading