New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simple Command Line Interface for JaCoCo #525

Merged
merged 34 commits into from May 24, 2017

Conversation

Projects
None yet
4 participants
@marchof
Member

marchof commented Apr 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Apr 28, 2017

Member

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

Member

marchof commented Apr 28, 2017

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

@marchof marchof self-assigned this Apr 28, 2017

@marchof marchof added this to the 0.7.10 milestone Apr 28, 2017

@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!

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 2, 2017

Member

@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.

Member

marchof commented May 2, 2017

@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.

Show outdated Hide outdated org.jacoco.build/pom.xml Outdated
Show outdated Hide outdated org.jacoco.doc/docroot/index.html Outdated
Show outdated Hide outdated org.jacoco.cli/.gitignore Outdated
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 8, 2017

Member

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.

Member

marchof commented May 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 9, 2017

Member

@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!

Member

marchof commented May 9, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mathjeff

mathjeff May 9, 2017

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!

mathjeff commented May 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 9, 2017

Member

@mathjeff Thanks for your feedback!

Member

marchof commented May 9, 2017

@mathjeff Thanks for your feedback!

marchof added some commits May 22, 2017

Added missing manifest headers.
Review comment by @Godin
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 22, 2017

Member

@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)
Member

marchof commented May 22, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 23, 2017

Member

@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.

Member

Godin commented May 23, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail May 24, 2017

Contributor

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

Contributor

bjkail commented May 24, 2017

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

marchof added some commits May 24, 2017

Simpify wording.
Review comment by @Godin
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 24, 2017

Member

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

Member

marchof commented May 24, 2017

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

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 24, 2017

Member

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

Member

marchof commented May 24, 2017

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

marchof and others added some commits May 24, 2017

@Godin

Godin approved these changes May 24, 2017

@Godin Godin merged commit 13f12d3 into master May 24, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Godin Godin deleted the issue-525 branch May 24, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.