Simple Command Line Interface for JaCoCo #525

marchof · 2017-04-28T21:45:51Z

A feature often asked for is a command line interface for JaCoCo. We have a pull request (#86) since quite some time, unfortunately without any tests and documentation. This is a new approach based on some ideas of #86.

To clearly manage expectations: The scope is about simple use cases only! For complex scenarios we have plugins to all common build systems.

Open Issues

Clarify and test usage of multi value options (e.g. -classfiles)
Improve documentation layout (long usage lines)
Test class XmlDocumenation or remove from test scope
Clarify class file version issue in build with exec-maven-plugin:1.6.0
Clarify why build result gets too big when compiled with Java 9

marchof · 2017-04-28T21:51:55Z

@mathjeff Can you please take a look and check whether your scenario is covered by this new PR? What is missing?

mathjeff

Hey thanks. This looks to me like most of what Android is looking for.

I think the main thing left to do before Android is able to use this is for classname filtering to be supported in the 'instrument' and 'report' goals similarly to what the Maven plugin does. The main reason that this would be good for Android is that it enables filtering out dependencies in a manner that's approximately api-compatible with EMMA, rather than having to move or copy all of the relevant class files into a separate directory.

The usage I imagine would be as in mathjeff@0e0c5ec#diff-eec9564b9926238a12deea932c4d57f6 with "-include" and "-exclude" options that filter on fully qualified classname.

Although it sounds just as reasonable to me for filtering to be part of a separate pull request.

Thanks!

mathjeff · 2017-05-01T21:17:41Z

org.jacoco.cli.test/src/org/jacoco/cli/internal/commands/ReportTest.java

+	}
+
+	@Test
+	public void shouldCreateCsvReport_whenXmlOptionIsProvided()


"Xml" -> "Csv"

@mathjeff Fixed.

mathjeff · 2017-05-01T21:19:49Z

org.jacoco.cli/src/org/jacoco/cli/internal/commands/Instrument.java

+ */
+public class Instrument extends Command {
+
+	@Option(name = "-destdir", usage = "directory to write instrumented Java classes to", metaVar = "<dir>", required = true)


Could we remove the 'dir' from '-destdir', possibly calling it '-destpath', to clarify the case where the input files is a .jar file and the value of '-destdir' is a .jar file?

@mathjeff Correct, -dest is the way to go.

@mathjeff Fixed.

mathjeff · 2017-05-01T21:22:59Z

org.jacoco.cli/src/org/jacoco/cli/internal/Main.java

+			command.printHelp(out);
+			return 0;
+		} else {
+			return command.execute(out, err);


We'll want to flush out and err somewhere, probably here; I was surprised that "[INFO] %s classes instrumented to %s.%n" wasn't getting displayed when I tested this. Of course, it's more convenient for Android if the output is empty, although I'm thinking that either we'll add a '-quiet' option to Jacoco or we'll redirect the output within Android.

@mathjeff Not sure what the problem here is: in Main.main() the PrintWriters are created with auto-flushing after every line. How do you reproduce this?

Yeah I was surprised too. The simplest example I have is to ask jacoco to instrument itself:

/workspace/jacoco/org.jacoco.cli (work)$ java -jar target/org.jacoco.cli-0.7.10-SNAPSHOT-nodeps.jar instrument target/org.jacoco.cli-0.7.10-SNAPSHOT.jar -destdir /tmp/dummy.jar

, which doesn't give any output when I run it.

@marchof actually as far as I can see currently main does new PrintWriter(System.out) and JavaDoc says :

Creates a new PrintWriter, without automatic line flushing

@mathjeff @Godin May bad -- missread JavaDoc. Fixed ✅

@mathjeff -quiet option is now in place.

mathjeff · 2017-05-01T21:24:58Z

org.jacoco.cli.test/pom.xml

+          <artifactId>*</artifactId>
+        </exclusion>
+      </exclusions>
+       -->


Is this intentional?

@mathjeff I tried to directly test the "shaded" version. Unfortunatelly this does not work within the m2e IDE. Should be be removed of course.

@mathjeff Fixed.

marchof · 2017-05-02T13:41:43Z

@mathjeff Hi, thanks for the detailled review! I will adress the issues you observed.

Regarding filtering: I would prefer to handle this in a separate PR as it should then be consistently implemented for the other integrations. Also the core API should be improved for this.

Godin · 2017-05-02T21:08:08Z

org.jacoco.build/pom.xml

+        <plugin>
+          <groupId>org.codehaus.mojo</groupId>
+          <artifactId>exec-maven-plugin</artifactId>
+          <version>1.6.0</version>


@marchof so far our lower bound of Maven version for build of project - is 3.0, which in his turn has Java 5 as lower bound, while exec-maven-plugin version 1.6.0 requires Java 7. This explains failure in Travis where we use JDK 6 to run Maven.

@Godin Ok, thanks for the Info! We can execute the documentation as part of the test. Or is that too much of a hack?

@marchof why not simply use 1.5.0 version of exec-maven-plugin that requires Java 5?

@Godin ✅ Done.

Godin · 2017-05-06T22:31:17Z

org.jacoco.doc/docroot/index.html

@@ -68,6 +68,12 @@
      <td>Ant <i>(all other dependencies included)</i></td>
    </tr>
    <tr>
+      <td><span class="el_jar">jacococli.jar</span></td>
+      <td>no</td>
+      <td>JaCoCo Ant tasks</td>


@marchof copy-paste detected 😉

@Godin Thanks, fixed!

Godin · 2017-05-06T22:32:14Z

org.jacoco.cli/.gitignore

@@ -0,0 +1,2 @@
+/target
+/bin


@marchof I don't think this is needed - we have consolidated .gitignore

@Godin Thanks, fixed! (started this branch before we consolidates .gitignore)

marchof · 2017-05-08T15:54:08Z

Resulting jacoco-xxx.zip files for different build:

ZIP size	master	cli
Java 5	3,035,510	3,523,558
Java 9	3,462,577	3,956,019

Looks like we have constant overheads which sums up.

master -> cli: 475 KB

This is primarly due to a new all-in JAR and bigger coverage and test report.

Java 5 -> Java 9: 417 KB

The reason here is that the JavaDoc grows from 2,8MB to 4,1MB, probably as JavaDoc created with Java 9 comes with new features and bundles JQuery.

For Java 9 and cli both effects add-up beyond our current check limit.

As this is a pure sanity check I think we can safely lift the upper limit a bit.

marchof · 2017-05-09T16:30:46Z

@Godin @mathjeff From my point of view this feature is ready to merge. If you find some time please double check and approve the review if you agree. Thx!

mathjeff · 2017-05-09T20:03:25Z

Hey thanks marchof@ for spending the time on this. Based on reading the code and trying out the resultant jar, this looks good to me. I'll leave the final approval/disapproval up to Godin@.

Android won't be able to directly use this yet until filtering by classname is implemented in a subsequent pull request, but this pull request looks consistent and reasonable to me.

Thanks!

marchof · 2017-05-09T20:07:32Z

@mathjeff Thanks for your feedback!

Otherwise output is not visible on the console.

@Godin

Review comment by @Godin

@Godin

Review comment by @Godin

@Godin

Review comment by @Godin

@Godin

Review comment by @Godin

@Godin

Review comment by @Godin

@Godin

Review comment by @Godin

@Godin

Review comment by @Godin

marchof · 2017-05-22T23:09:10Z

@Godin Thanks for taking the time to perform such a detailed review! You identified quite a few relevant issues. I hope for every review remark I either fixed the issue or left a comment when I was unsure how to proceed with it.

For me the remaining main issue is how the multi value arguments execfiles and classfiles should behave when no value is provided at all. There are several options:

Fail (make them mandatory). This may exclude some use cases for the commands.
Print warning (might be suppressed with --quiet
Consider this as valid usage (status quo)

Godin · 2017-05-23T17:50:31Z

org.jacoco.cli.test/src/org/jacoco/cli/internal/commands/ReportTest.java

+		execute("report", "--classfiles", getClassPath());
+
+		assertOk();
+		assertContains("[INFO] Writing report with 14 classes.", out);


@marchof as discussed in #525 (comment) - Analyzing 14 classes. sounds better

@Godin Done in baaf539 ✅

Godin · 2017-05-23T18:31:17Z

org.jacoco.doc/docroot/doc/integrations.html

-      <td><a href="agent.html">JaCoCo Manual</a></td>
-      <td></td>
+      <td><a href="agent.html">JaCoCo Java agent</a>, <a href="cli.html">JaCoCo Command Line Interface</a></td>
+      <td>Since version 0.8.0</td>


@marchof given that version is only about CLI, I think will be better to have separate entries for Agent and CLI

@Godin Fixed in 8cd9627 ✅

Godin · 2017-05-23T19:04:55Z

@marchof Re multi value arguments: the use case that you described in #525 (comment) doesn't work as is for me:

$ bash
$ java -jar jacococli.jar merge -destfile /tmp/jacoco.exec testclients/*.exec
[INFO] Loading execution data file /private/tmp/j/testclients/*.exec.
Exception in thread "main" java.io.FileNotFoundException: testclients/*.exec (No such file or directory)

$ exit

$ zsh
$ java -jar jacococli.jar merge --destfile /tmp/jacoco.exec testclients/*.exec
zsh: no matches found: testclients/*.exec

$ exit

And I personally was puzzled multiple times by empty output in console after forgetting to provide an argument:

$ java -jar jacococli.jar classinfo
$ java -jar jacococli.jar execinfo

And by an empty report after forgetting to provide exec files:

java -jar jacococli.jar report --classfiles jacococli.jar --html report

Hence warned that potentially this might become a source of common mistakes.
An empty report after merge also might become a source of common mistakes:

java -jar jacococli.jar merge --destfile merged.exec
java -jar jacococli.jar report --classfiles jacococli.jar --html report merged.exec

For the instrument there is at least output for the ones, who are capable of reading it 😆

java -jar jacococli.jar instrument --dest instrumented
[INFO] 0 classes instrumented to /private/tmp/j/instrumented.

So printing to console sounds like a good option.
But I'm fine to leave this as is for now.

Godin

@marchof just few tiny remaining things to address:

and please respond on #525 (comment)

bjkail · 2017-05-24T03:06:48Z

@Godin The nullglob shell option should make it work, but it's not the default.

@Godin

Review comment by @Godin

@Godin

Review comment by @Godin

@Godin

Review comment by @Godin

marchof · 2017-05-24T11:27:24Z

@Godin Added warning about not providing exec files or class files to merge, report, classinfo and execinfo in 4907b22 ✅

@Godin

Review comment by @Godin

marchof · 2017-05-24T11:46:07Z

@Godin I think I now fixed all issues identified by you. Thx again for the detailed review!

marchof added type: enhancement component: cli labels Apr 28, 2017

marchof added a commit that referenced this pull request Apr 28, 2017

Github #525: Simple Command Line Interface

d850ed4

marchof mentioned this pull request Apr 28, 2017

Add CLI tools for offline instrumentation and report generation #86

Closed

marchof self-assigned this Apr 28, 2017

marchof added this to the 0.7.10 milestone Apr 28, 2017

mathjeff reviewed May 1, 2017

View reviewed changes

Godin reviewed May 2, 2017

View reviewed changes

Godin reviewed May 6, 2017

View reviewed changes

marchof mentioned this pull request May 12, 2017

include commandline tool for merge/reporting of jacoco results in main source tree/final jars #50

Closed

marchof added 13 commits May 18, 2017 21:44

Github #525: Simple Command Line Interface

ebbc796

Remove obsolete, commented configuration.

88f7c23

Correct test method naming.

92f784e

Use -dest for option name as output can be folder or file.

069decb

Flush output automatically

3616609

Otherwise output is not visible on the console.

Make sure the build runs with Java 5.

9819173

Wrap long usage lines in documentation.

d238d68

Close ServerSocket in tests.

c535065

Update documentation for command line interface

a00f1d4

Command to print version.

15fa090

Adjust test case names to new conventions.

4b0aaa5

Test and documentation for multiple values.

5c33bf3

Tests for XML documentation generator.

b7b48fc

marchof added 7 commits May 22, 2017 21:07

Exit with calculated return code.

1a8ad0f

Review comment by @Godin

Default value consistent with Ant and Maven.

c3e2bc9

Review comment by @Godin

Warning about offline instrumentation.

f1d8d80

Review comment by @Godin

Use JEP 293 compliant double dashes for options.

494c8f5

Review comment by @Godin

Improve help and error message formatting

1dd730d

Review comment by @Godin

Added missing manifest headers.

920c3cf

Review comment by @Godin

Don't include version of third party libraries

601b2d5

Review comment by @Godin

Godin mentioned this pull request May 23, 2017

ExecDumpClient should report error when no execution data is retrieved #538

Merged

This was referenced May 23, 2017

Consistent license infos #539

Closed

Update documentation for published Maven artifacts #540

Merged

Godin reviewed May 23, 2017

View reviewed changes

Godin requested changes May 23, 2017

View reviewed changes

marchof added 3 commits May 24, 2017 12:41

Simpify wording.

5df46d8

Review comment by @Godin

Depending on configuration report might not be written.

baaf539

Review comment by @Godin

Print warning if no exec files are provided to avoid common mistakes.

4907b22

Review comment by @Godin

marchof force-pushed the issue-525 branch from e0af07b to 4907b22 Compare May 24, 2017 11:25

Separate agent and CLI due to different since version.

8cd9627

Review comment by @Godin

marchof and others added 2 commits May 24, 2017 14:59

Revert changes now addressed in #538.

4115a6e

Merge branch 'master' into issue-525

eff97dc

Godin approved these changes May 24, 2017

View reviewed changes

Godin merged commit 13f12d3 into master May 24, 2017

Godin deleted the issue-525 branch May 24, 2017 22:53

Godin mentioned this pull request Jul 10, 2017

Relocate 3rd party libs in CLI jar #561

Merged

jacoco locked as resolved and limited conversation to collaborators Jan 11, 2018

		@@ -0,0 +1,2 @@
		/target
		/bin

Simple Command Line Interface for JaCoCo #525

Simple Command Line Interface for JaCoCo #525

Conversation

marchof commented Apr 28, 2017 • edited Loading

Open Issues

marchof commented Apr 28, 2017

mathjeff 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchof commented May 2, 2017

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

marchof commented May 8, 2017

marchof commented May 9, 2017

mathjeff commented May 9, 2017

marchof commented May 9, 2017

marchof commented May 22, 2017

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

Godin commented May 23, 2017

Godin left a comment

Choose a reason for hiding this comment

bjkail commented May 24, 2017

marchof commented May 24, 2017

marchof commented May 24, 2017

marchof commented Apr 28, 2017 •

edited

Loading