Introduce task for JaCoCo plugin for enforcing code coverage metrics #983

bmuschko · 2016-12-07T17:30:21Z

Please see issue #824 for more information.

Design decisions to consider when reviewing:

Configuration options have been aligned with terminology from the JaCoCo document to allow users to pin back options to Gradle plugin.
Pre-defined values for certain configuration options have not been represented by enums. This is to ensure best forward and backward compatibility of JaCoCo versions without having to change plugin code.
Violation checks have been implemented as separate task to ensure fine-grained control for the end user. The representing task has not be hooked up with check to avoid strong coupling.

Allows for better forward and backward capability in case JaCoCo decides to introduce new values or change existing ones.

Makes it easier for users to map configuration options to Jacoco documentation. Aligns the configuration options with Maven plugin and Ant tasks.

Results in better configurability for end user and decouples report generation from coverage verification.

lptr · 2016-12-07T17:55:17Z

subprojects/jacoco/src/main/java/org/gradle/testing/jacoco/tasks/JacocoCheck.java

+ *
+ * @since 4.0
+ */
+@CacheableTask


IIUC this task declares no output, and thus won't ever be cached.

eriwen · 2016-12-07T18:35:12Z

I'll volunteer to do this review.

eriwen

Overall changes look very good. I have noted 2 things to reconsider: A TODO comment and the jacocoTestCheck task name.

I really appreciate you breaking this change into many small commits — that allowed me to follow along very well and I think made for a more effective review.

eriwen · 2016-12-19T21:27:58Z

subprojects/jacoco/src/main/java/org/gradle/testing/jacoco/tasks/JacocoReportBase.java

+        onlyIf(new Spec<Task>() {
+            @Override
+            public boolean isSatisfiedBy(Task element) {
+                //TODO SF it should be 'any' instead of 'all'


We should address this TODO comment or remove it.

eriwen · 2016-12-19T21:33:43Z

subprojects/docs/src/docs/userguide/jacocoPlugin.xml

@@ -236,6 +241,16 @@
                </td>
                <td>Generates code coverage report for the test task.</td>
            </tr>
+            <tr>
+                <td>
+                    <literal>jacocoTestCheck</literal>


Nitpick: this task name doesn't represent what it's for IMO. Perhaps jacocoVerificationCheck or jacocoEnforceThresholds or something.

eriwen · 2016-12-19T21:36:48Z

subprojects/jacoco/src/main/java/org/gradle/testing/jacoco/plugins/JacocoPlugin.java

@@ -247,4 +249,10 @@ public File call() {
            }
        });
    }
+
+    private void addDefaultCheckTask(final Test task) {
+        final JacocoCheck checkTask = project.getTasks().create("jacoco" + StringUtils.capitalise(task.getName()) + "Check", JacocoCheck.class);


Same nitpick as elsewhere: jacocoTestCheck seems like a confusing name to me. What name would make it very obvious what it's purpose was?

eriwen · 2016-12-19T21:40:03Z

subprojects/jacoco/src/main/java/org/gradle/testing/jacoco/tasks/rules/JacocoViolationRule.java

@@ -31,13 +34,14 @@
 * @since 4.0
 */
 @Incubating
-public interface JacocoViolationRule {
+public interface JacocoViolationRule extends Serializable {


Silly question: why does this need to be Serializable?

eriwen · 2016-12-19T21:42:30Z

.../src/integTest/groovy/org/gradle/testing/jacoco/plugins/fixtures/JavaProjectUnderTest.groovy

+
+import org.gradle.test.fixtures.file.TestFile
+
+class JavaProjectUnderTest {


This is a really cool idea. We should do this elsewhere (not a part of this PR of course).

We do this in other places already. Usually, they're called _Something_App. Look at all of the TestApp classes or PlayApp for other examples.

eriwen · 2016-12-19T22:15:04Z

subprojects/jacoco/src/main/java/org/gradle/testing/jacoco/plugins/JacocoPlugin.java

@@ -247,4 +249,10 @@ public File call() {
            }
        });
    }
+
+    private void addDefaultCheckTask(final Test task) {
+        final JacocoCheck checkTask = project.getTasks().create("jacoco" + StringUtils.capitalise(task.getName()) + "Check", JacocoCheck.class);


Seems like we should add these jacoco* tasks to a group so they're listed in gradle tasks report

done. Also added a description.

eriwen · 2016-12-20T17:20:16Z

@bmuschko We may need to handle Java 9 in tandem with this. Nearly all of the tests failed for JaCoCo on Java 9: https://builds.gradle.org/viewLog.html?buildTypeId=Gradle_Branches_CoveragePhase_LinuxCoverage_LinuxJava19_8LinuxJava19&buildId=1958128

wolfs · 2016-12-20T17:57:09Z

@eriwen That is just the case since all Jacoco tests are disabled on Java 9. Just the new tests have not yet been ignored. Jacoco support for Java 9 would be handled by #1006.

bmuschko · 2016-12-20T18:12:54Z

@eriwen I'll ignore running those tests on Java 9 for now and will address Java 9 support separately.

eriwen

LGTM!

big-guy

🎅

big-guy · 2016-12-22T21:14:35Z

...jacoco/src/integTest/groovy/org/gradle/testing/jacoco/plugins/fixtures/JacocoCoverage.groovy

+        jacocoVersion.compareTo(supportedJacocoVersion) == -1
+    }.asImmutable()
+
+    private static class JacocoVersion implements Comparable<JacocoVersion> {


can we reuse VersionNumber? You'd get a Comparable for free.

In the spirit of dog-fooding I'd like to avoid using the internal class VersionNumber. That'll also make it easier to external the plugin later.

big-guy · 2016-12-22T21:15:05Z

...jacoco/src/integTest/groovy/org/gradle/testing/jacoco/plugins/fixtures/JacocoCoverage.groovy

+    }.asImmutable()
+
+    private static class JacocoVersion implements Comparable<JacocoVersion> {
+        final static CHECK_INTRODUCED = new JacocoVersion(0, 6, 3)


Can this be put outside of JacocoVersion?

That's a "pattern" described in Effective Java on page 76, chapter 4. What would be benefit of moving it out?

nevermind. It only made sense if we were replacing JacocoVersion for VersionNumber.

big-guy · 2016-12-22T21:18:21Z

.../src/integTest/groovy/org/gradle/testing/jacoco/plugins/fixtures/JavaProjectUnderTest.groovy

+
+import org.gradle.test.fixtures.file.TestFile
+
+class JavaProjectUnderTest {


We do this in other places already. Usually, they're called _Something_App. Look at all of the TestApp classes or PlayApp for other examples.

big-guy · 2016-12-22T21:18:43Z

...g/jacoco/plugins/rules/AbstractJacocoPluginCoverageVerificationVersionIntegrationTest.groovy

+            jacocoTestCoverageVerification {
+                violationRules {
+                    rule {
+                        $Sufficient.LINE_METRIC_COVERED_RATIO


I get a weird IntelliJ warning about this and the import above. Do you?

I've seen this before. Interestingly, it works if I import the class again - but the file didn't actually change. This looks like a bug in IntelliJ to me.

big-guy · 2016-12-22T21:19:43Z

...g/jacoco/plugins/rules/AbstractJacocoPluginCoverageVerificationVersionIntegrationTest.groovy

+abstract class AbstractJacocoPluginCoverageVerificationVersionIntegrationTest extends MultiVersionIntegrationSpec {
+
+    private final JavaProjectUnderTest javaProjectUnderTest = new JavaProjectUnderTest(testDirectory)
+    protected final static String[] TEST_AND_JACOCO_COVERAGE_VERIFICATION_TASK_PATHS = [':test', ':jacocoTestCoverageVerification'] as String[]


Is it not enough to just run with :jacocoTestCoverageVerification?

The task :jacocoTestCoverageVerification doesn't have any task dependencies. Without running the test task first, we wouldn't have any execution data and therefore the verification would be SKIPPED.

big-guy · 2016-12-22T21:38:02Z

subprojects/jacoco/src/main/java/org/gradle/testing/jacoco/tasks/rules/package-info.java

+ */
+
+/**
+ * Implementations for Jacoco code coverage rules.


Might be helpful to provide links to Jacoco's docs on this (if there are any)

I think it would feel kind of weird to link to the documentation page of the Ant task. That documentation page also doesn't have any anchors so users would have to scroll down to even find what they are looking for. It would probably confuse more than help.

big-guy · 2016-12-22T21:38:55Z

subprojects/jacoco/src/main/java/org/gradle/testing/jacoco/tasks/rules/JacocoViolationRule.java

+     * Gets all limits defined for this rule. Defaults to an empty list.
+     */
+    @Nested
+    List<JacocoLimit> getLimits();


I don't think this works (but I would need to check...). I think we would end up looking for annotations on List.

Good point. Will use @Input instead.

big-guy · 2016-12-22T21:41:32Z

subprojects/docs/src/docs/userguide/jacocoPlugin.xml

+        </para>
+        <sample id="configViolationRules" dir="testing/jacoco/quickstart" title="Configuring violation rules">
+            <sourcefile file="build.gradle" snippet="violation-rules-configuration"/>
+        </sample>


Are there any Jacoco docs we could link to too?

See comment above.

big-guy · 2016-12-22T21:43:42Z

subprojects/jacoco/src/main/java/org/gradle/testing/jacoco/tasks/rules/JacocoViolationRule.java

+    /**
+     * Gets the element for the rule as defined by
+     * <a href="http://www.eclemma.org/jacoco/trunk/doc/api/org/jacoco/core/analysis/ICoverageNode.ElementType.html">org.jacoco.core.analysis.ICoverageNode.ElementType</a>.
+     * Valid scope values are BUNDLE, PACKAGE, CLASS, SOURCEFILE and METHOD. Defaults to BUNDLE.


Is there any kind of precedence here? e.g., if I define a limit for PACKAGE and another limit for CLASS, does one override the other?

Not sure about that. IMHO that's something the Ant task should handle for us.

big-guy · 2016-12-22T21:45:56Z

subprojects/jacoco/src/main/java/org/gradle/testing/jacoco/tasks/rules/JacocoViolationRule.java

+    @Input
+    List<String> getIncludes();
+
+    void setExcludes(List<String> excludes);


We need javadoc on the setters.

Seems to be a pattern we use across various plugins e.g.

https://github.com/gradle/gradle/blob/master/subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/FindBugsExtension.java#L94

https://github.com/gradle/gradle/blob/master/subprojects/jacoco/src/main/java/org/gradle/testing/jacoco/plugins/JacocoPluginExtension.java#L81

That's why I didn't add it.

Benjamin Muschko added 30 commits December 1, 2016 18:15

Basic support for enforcing Jacoco metrics

313367a

Add test cases for multiple report tasks

d7f3b7d

Add test case for multiple rules

3f50fcd

Break out method

5c3a8ad

Expose configuration option for disabling rules

c27a0b2

Rules can be checked even if all report formats are disabled

3f971f7

Use strong typing for enums

2c7be9f

Rename of method

530828f

Add some unit tests

493e282

Renamed property

317ec14

Set default values for fields as described in Jacoco docs

5b0f085

Renamed fields to better express intention

f8f3070

Rename field

9b5ed0a

Enhance samples and documentation

358a6f9

Better IDE support for Closure parameters

a73a193

Bump up version number

6627b49

Use String data type instead of enum

8e6d20c

Allows for better forward and backward capability in case JaCoCo decides to introduce new values or change existing ones.

Align terminology with Jacoco configuration options

70b0f0a

Makes it easier for users to map configuration options to Jacoco documentation. Aligns the configuration options with Maven plugin and Ant tasks.

Apply input/output annotations

aed27f6

Document default value for property

00b2cb3

Verify proper behavior of Ant task for unknown values

0dc8b31

Remove spaces

45674bd

Add multi-version coverage for check support

ebb4d11

Reuse version number across all multi-version tests

41dcf72

Add missing word

0521afb

Use constant for task paths

6c63ff9

Extract dedicated task for verifying if coverage metrics are met

db94948

Results in better configurability for end user and decouples report generation from coverage verification.

Input annotation make sure that requires properties are not null

f024a94

Reflect use of JacocoCheck task in documentation and sample

aa402c8

Revert change

08882e9

bmuschko added a:feature A new functionality from:member in:jacoco-plugin labels Dec 7, 2016

lptr reviewed Dec 7, 2016

View reviewed changes

eriwen self-assigned this Dec 7, 2016

Benjamin Muschko added 2 commits December 7, 2016 14:02

Move logic into parent class

47feb94

Task is not cacheable as it doesn't declare outputs

8097fdd

eriwen self-requested a review December 12, 2016 21:58

eriwen assigned bmuschko and unassigned eriwen Dec 12, 2016

eriwen suggested changes Dec 19, 2016

View reviewed changes

eriwen reviewed Dec 19, 2016

View reviewed changes

Benjamin Muschko added 6 commits December 20, 2016 11:13

Ignore running tests on Java 9

ff77afc

Report task should fail if any of the input file does not exist

1e31712

Assign group and description for tasks

a7a4592

Use task name that better expresses intent

d900795

Change targeted release version

d832d0d

Fix task name in sample project

ec1b003

eriwen approved these changes Dec 20, 2016

View reviewed changes

bmuschko added this to the 4.0 milestone Dec 20, 2016

bmuschko merged commit c7abc17 into master Dec 21, 2016

bmuschko deleted the bm-jacoco-enforce-metrics branch December 21, 2016 15:25

big-guy reviewed Dec 22, 2016

View reviewed changes

bmuschko mentioned this pull request Jan 3, 2017

Polishing of JaCoCo coverage validation changes #1091

Merged

bmuschko modified the milestones: 3.4 RC1, 4.0 Jan 10, 2017


		import org.gradle.test.fixtures.file.TestFile

		class JavaProjectUnderTest {

Introduce task for JaCoCo plugin for enforcing code coverage metrics #983

Introduce task for JaCoCo plugin for enforcing code coverage metrics #983

Conversation

bmuschko commented Dec 7, 2016 • edited by eriwen

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriwen commented Dec 7, 2016

eriwen 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

Choose a reason for hiding this comment

eriwen commented Dec 20, 2016

wolfs commented Dec 20, 2016

bmuschko commented Dec 20, 2016

eriwen left a comment

Choose a reason for hiding this comment

big-guy 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmuschko Jan 3, 2017 • edited

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

bmuschko commented Dec 7, 2016 •

edited by eriwen

bmuschko Jan 3, 2017 •

edited