Add Jacoco Coverage Provider #161

ChaitanyaPramod · 2016-06-19T09:05:32Z

Fixes #99

ChaitanyaPramod · 2016-06-19T09:31:47Z

Will reopen after fixing test

kageiit · 2016-10-27T22:22:36Z

@ChaitanyaPramod what happened to this PR?

coveralls · 2018-03-30T22:54:35Z

Coverage decreased (-0.2%) to 90.464% when pulling 6277313 on ChaitanyaPramod:add_jacoco_support into 6a1010c on uber:master.

kageiit · 2018-03-30T22:57:00Z

Thanks for reviving this PR!

kageiit · 2018-03-30T22:58:00Z

phabricator-plugin.iml

    <orderEntry type="library" name="Maven: net.sourceforge.nekohtml:nekohtml:1.9.13" level="project" />
    <orderEntry type="library" name="Maven: net.sf.trove4j:trove4j:3.0.3" level="project" />
+    <orderEntry type="library" name="Maven: org.jenkins-ci.plugins:jacoco:1.0.19" level="project" />
+    <orderEntry type="library" name="Maven: org.jacoco:org.jacoco.report:0.7.4.201502262128" level="project" />


Do you mind using jacoco 0.8.1 which is the latest stable

kageiit · 2018-03-30T22:58:20Z

phabricator-plugin.iml

+    <orderEntry type="library" name="Maven: org.jacoco:org.jacoco.core:0.7.4.201502262128" level="project" />
+    <orderEntry type="library" name="Maven: org.ow2.asm:asm-debug-all:5.0.1" level="project" />
+    <orderEntry type="library" scope="RUNTIME" name="Maven: org.kohsuke:asm5:5.0.1" level="project" />
+    <orderEntry type="library" scope="RUNTIME" name="Maven: org.ow2.asm:asm:5.0.1" level="project" />


latest jacoco requires asm 6 for java 9/10 support

kageiit

please address comments

ChaitanyaPramod · 2018-03-31T05:48:25Z

build.gradle

  optionalJenkinsPlugins 'org.jenkins-ci.plugins:junit:1.6@jar'
  optionalJenkinsPlugins 'org.jenkins-ci.plugins:cobertura:1.9.6@jar'
+  optionalJenkinsPlugins 'org.jenkins-ci.plugins:jacoco:2.0.1@jar'
+  providedCompile 'org.jacoco:org.jacoco.report:0.7.5.201505241946'


@kageiit

Do you mind using jacoco 0.8.1 which is the latest stable

latest jacoco requires asm 6 for java 9/10 support

Jacoco is on the compile classpath only so compiler can find its symbols for compilation. Jacoco, at runtime, comes from Jacoco Jenkins plugin. Updating Jacoco, therefore, would be out of this PR's scope and would belong in the Jacoco plugin. Jacoco plugin 3.0.1 uses Jacoco 0.8.0 and ASM 6.

Sounds good

ChaitanyaPramod · 2018-03-31T10:41:05Z

src/main/java/com/uber/jenkins/phabricator/coverage/JacocoCoverageProvider.java

+                fakeHitCount = 1;
+                break;
+            case ICounter.PARTLY_COVERED:
+                fakeHitCount = 1; // Harbormaster format doesn't allow for partially covered lines


Consider partially covered lines as uncovered instead

ChaitanyaPramod · 2018-03-31T10:52:37Z

src/test/java/com/uber/jenkins/phabricator/coverage/JacocoCoverageProviderTestWithJenkins.java

+    }
+
+    @Test
+    @Ignore


Finish this test

kageiit · 2018-06-12T14:23:47Z

@ChaitanyaPramod plans to finish the test in the PR? Planning to cut a release soon and would be good to get this merged before that

ChaitanyaPramod · 2018-06-17T18:10:33Z

Finally in working state.

I'd like the test finished before merging, would give me lot more peace of mind. Will not be able to spend more time on this until the next weekend. Contributions to tests are welcome. (I'd use uncovered lines as guide)

Would encourage community to try this out and report bugs, if any before merging.

kageiit · 2018-06-18T17:05:19Z

src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java

            return null;
        }

+        CoverageProvider coverage = Objects.firstNonNull(coberturaCoverage, jacocoCoverage);


Im not sure this logic is sound. Cobertura plugin could be available but only jacoco report could have been generated. We want to select the provider based on which one has coverage available.

Ideally we would support a hybrid coverage provider that can aggregate coverage results from all available providers. This can be a follow up/enhancement however

kageiit · 2018-06-18T17:06:53Z

src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java

+        CoverageProvider jacocoCoverage = jacocoProvider.getInstance();

-        if (coverage == null) {
+        if (coberturaCoverage == null && jacocoCoverage == null) {


lets pick the first non null coverage provider that satisfies

coverage.hasCoverage() == true

kageiit · 2018-06-18T17:19:14Z

src/main/java/com/uber/jenkins/phabricator/coverage/JacocoCoverageProvider.java

+        HashMap<String, List<Integer>> lineCoverage = new HashMap<String, List<Integer>>();
+
+        try {
+            ExecutionFileLoader executionFileLoader = jacocoAction.getJacocoReport().parse(null, null);


If we only need the getJacocoReport(), we should not be gating this feature on the presence of a jacoco build step. All we need is for the plugin to be available in jenkins so we have the runtime components to parse a coverage result. We should be able to use it to parse the result and provide coverage. The cobertura provider does not need the cobertura plugin to be applied to the job either, so we should mirror the same behavior by adding a config option to the phab jenkins plugint to accept the path to the jacoco xml report.

We pick a few things from the JacocoPublisher attached to the current build, so we can't do that.
That said, I prefer we take advantage of Jenkins ecosystem and not recreate the coverage plugin's function here. It's trivial to add a coverage plugin to a job and we get to maintain minimal code here.

No worries. Feel free to leave it this way. We will add a way to not have the jacoco plugin added to the job and still use the functionality in a separate change.

ChaitanyaPramod · 2018-06-25T05:10:28Z

The Java 7 test fails on CI, but works locally. The CI uses older version of Java (1.7.0_121) which doesn't have TLS 1.2 enabled by default, and hence is not able to connect to now TLS 1.2 only maven repositories.
The trusty images have newer Java but that seems to be a problem for us (#237) as well.

kageiit

Couple of minor nits to address. LGTM after that

kageiit · 2018-06-25T14:40:02Z

src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java

-import java.util.HashSet;
-import java.util.List;
-import java.util.Set;
+import java.util.*;


can you please avoid star imports?

You may want to configure your IDE to not do this

kageiit · 2018-06-25T14:42:00Z

src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java

+                i.remove();
+            }
+        }
+        if (!coverageProviders.isEmpty()) {


Can we add a logger.info statement here about which coverage we picked if its a non empty list?

It will help with debugging issues

ChaitanyaPramod · 2018-09-10T07:01:45Z

@kageiit's earlier concerns have been addressed and the PR is ready for review.

PS: I think we should close #266 before merging the other PRs

kageiit · 2018-09-10T13:58:21Z

@ChaitanyaPramod ci issues should be fixed. Can you please rebase?

ChaitanyaPramod · 2018-09-11T04:11:30Z

@kageiit Rebased, and the CI build has succeeded.

ChaitanyaPramod closed this Jun 19, 2016

ChaitanyaPramod reopened this Mar 30, 2018

ChaitanyaPramod force-pushed the add_jacoco_support branch 2 times, most recently from 68ca588 to 81b84aa Compare March 30, 2018 22:52

kageiit reviewed Mar 30, 2018

View reviewed changes

kageiit suggested changes Mar 30, 2018

View reviewed changes

ChaitanyaPramod force-pushed the add_jacoco_support branch from 81b84aa to fde8c78 Compare March 31, 2018 05:12

ChaitanyaPramod commented Mar 31, 2018

View reviewed changes

ChaitanyaPramod force-pushed the add_jacoco_support branch from fde8c78 to 8476a3f Compare March 31, 2018 13:22

ChaitanyaPramod force-pushed the add_jacoco_support branch from 8476a3f to ea5b5e7 Compare June 17, 2018 18:01

ChaitanyaPramod force-pushed the add_jacoco_support branch from ea5b5e7 to 3dd6811 Compare June 17, 2018 18:33

kageiit suggested changes Jun 18, 2018

View reviewed changes

kageiit reviewed Jun 18, 2018

View reviewed changes

kageiit suggested changes Jun 18, 2018

View reviewed changes

ChaitanyaPramod force-pushed the add_jacoco_support branch 2 times, most recently from 68f5d87 to b6b328f Compare June 24, 2018 17:05

kageiit suggested changes Jun 25, 2018

View reviewed changes

ChaitanyaPramod force-pushed the add_jacoco_support branch from b6b328f to adeed25 Compare June 26, 2018 03:05

ChaitanyaPramod mentioned this pull request Aug 11, 2018

Travis build for java7 fails #266

Closed

Add Jacoco Coverage Provider

6277313

ChaitanyaPramod force-pushed the add_jacoco_support branch from adeed25 to 6277313 Compare September 10, 2018 16:05

kageiit approved these changes Sep 11, 2018

View reviewed changes

kageiit merged commit b8c9c1e into uber-archive:master Sep 11, 2018

kageiit mentioned this pull request Sep 11, 2018

Make Jacoco and Cobertura plugins no longer required #274

Closed

+                  }
+                  @Test
+                  @Ignore

Add Jacoco Coverage Provider #161

Add Jacoco Coverage Provider #161

Uh oh!

Conversation

ChaitanyaPramod commented Jun 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChaitanyaPramod commented Jun 19, 2016

Uh oh!

kageiit commented Oct 27, 2016

Uh oh!

coveralls commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kageiit commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kageiit left a comment

Choose a reason for hiding this comment

Uh oh!

ChaitanyaPramod Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kageiit commented Jun 12, 2018

Uh oh!

ChaitanyaPramod commented Jun 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kageiit Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kageiit Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChaitanyaPramod commented Jun 25, 2018

Uh oh!

kageiit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChaitanyaPramod commented Sep 10, 2018

Uh oh!

kageiit commented Sep 10, 2018

Uh oh!

ChaitanyaPramod commented Sep 11, 2018

Uh oh!

Uh oh!

ChaitanyaPramod commented Jun 19, 2016 •

edited

Loading

coveralls commented Mar 30, 2018 •

edited

Loading

ChaitanyaPramod Mar 31, 2018 •

edited

Loading

kageiit Jun 18, 2018 •

edited

Loading

kageiit Jun 25, 2018 •

edited

Loading