Include possibility of covering of boot-classpath #49
Conversation
|
This feature - implemented as mentioned here http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121127/ba8f6a1e/xboot-0001.patch - is now used by icedtea webin daily build and coverage of bootclasses is working pretty fine. I would like to point out that some classes have to be excluded. Eg icedtea-web run have this restrictions: Imho It is really important to mention that many classes must be excluded form processing: eg jacoco:java.lang.:java.reflect.:java.util.:sun.reflect. are imho enough, But one may need to make more precise exclusion. Loking forward to have this interesting feature in head! Best regards and tahnk you for your amazing work! |
|
hi! This issue is old, provided with full patch - http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121127/ba8f6a1e/xboot.patch - and still wonted. I need to recompile jacoco on my own after every update (yours or one of depndences) If you give me access, I will merge it to trunk. Otherwise, please, can we move wit this? |
|
@judovana Ok, I'll create a pull request from your patch for review this week. BTW you can create a pull request yourself -- no access needed. But I'll take care. |
|
Thank you very much! If it will be awesome. |
New configuration option for the JaCoCo agent includebootstrapclasses to also instrument classes from the bootstrap class loader.
|
@judovana I attached a pull request to this issue. Please feel free to comment on it.
|
|
Hi! I have checked a changeset and it looks ok. (I have no yet tried an build) Just for curiosity - when is planned next release? Thank yu very much!Mgr. Jiri Vanek ---------- Původní zpráva ---------- " @judovana(https://github.com/judovana) I attached a pull request to this
— " |
| * instrumented. Use this feature with caution, it needs heavy | ||
| * includes/excludes tuning. | ||
| * | ||
| * @parameter property="jacoco.includebootstrapclasses" |
There was a problem hiding this comment.
@marchof I believe that property name should be "jacoco.includeBootstrapClasses" in order to be consistent with naming of other properties
There was a problem hiding this comment.
When we are in this file, maybe tuning of " it needs heavy includes/excludes
tuning." to contain examples of
69
(http://icedtea.classpath.org/hg/icedtea-web/file/22761e42d5b3/Makefile.am#l69)
export JACOCO_BASE_EXCLUDE=org.junit.:junit.
70
(http://icedtea.classpath.org/hg/icedtea-web/file/22761e42d5b3/Makefile.am#l70)
export JACOCO_ADVANCED_EXCLUDE=:jacoco:java.lang.:java.reflect.:java.
util.:sun.reflect.
from (http://icedtea.classpath.org/hg/icedtea-web/file/22761e42d5b3/
Makefile.am#l69)
may be worthy.
Eg.: "it needs heavy includes/excludes. Most commonly excluded classes are :
jacoco:java.lang.:java.reflect.:java.util.:sun.reflect.. Also the
tesengine itself mostly needs excluding. Eg for junit it is list of org.
junit.:junit."
Just unblocking nit pick.
thank you again for push request!
J
---------- Původní zpráva ----------
Od: Evgeny Mandrikov notifications@github.com
Komu: jacoco/jacoco jacoco@noreply.github.com
Datum: 20. 5. 2014 11:23:08
Předmět: Re: [jacoco] Include possibility of covering of boot-classpath (#
49)
"
In jacoco-maven-plugin/src/org/jacoco/maven/AbstractAgentMojo.java:
@@ -73,6 +73,14 @@
/
String exclClassLoaders;
/*
- * Specifies whether also classes from the bootstrap classloader should be
- * instrumented. Use this feature with caution, it needs heavy
- * includes/excludes tuning.
- *
- * @parameter property="jacoco.includebootstrapclasses"
@marchof(https://github.com/marchof) I believe that property name should be
"jacoco.includeBootstrapClasses" in order to be consistent with naming of
other properties
—
Reply to this email directly or view it on GitHub
(https://github.com/jacoco/jacoco/pull/49/files#r12834237).
"
There was a problem hiding this comment.
@judovana I didn't understand your comment fully:
- how it relates to my comment about property "jacoco.includebootstrapclasses" ?
- what do you want to do with excludes? There is already a way to specify them. Or you want to have some sensible defaults?
There was a problem hiding this comment.
@judovana oh, sorry, "morning effect", got it - you want to update JavaDoc and mention some sensible defaults
|
@marchof what about addition of integration test for Maven Plugin to cover newly introduced property? |
Consistent and shorter naming.
|
Please note, unless you are covering JDK itself, you mostly need to excluded classes: :jacoco:java.lang.:java.reflect.:java.util.:sun.reflect.. Also the tesengine itself mostly needs excluding. Eg for junit it is list of org.junit.:junit." |
|
@Godin Of course integration tests would be nice. I added one for the Ant task. The tricky part is that you need to inspect the exec file and see whether classes from the bootclasspath have actually been instrumented. |
|
@marchof For Maven integration tests we already use BeanShell and can use Groovy, so it seems possible to look into content of exec. But why not - for the time being we can go ahead without this. |
|
@Godin So good to merge? |
Include possibility of covering of boot-classpath
Many projects are runnig some of theirs parts in boot classapth for various reasons.
Right now jacoco is exuding boot classpath explicitly.
I think it is worthy to include possibility to cover also bootclassapth, however with strong documentation (as some parts, eg jacoco or java.lang must be excluded otherwise fatal error or endless loop occure )and especially with "use on your own risk" :)
For my own purposes I have added this behaviour to jacoco -
http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121127/ba8f6a1e/xboot-0001.patch (for reasons - http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2012-November/020984.html) .
This proof of concept is very simple, it s adding new agent switch xboot=true/false default si false, and let user to include/exclude what he needs. Then it is just not excluding the boot classapth.
I think this improvement can be fast-done one and will bring enormous benefits. From above proof-of-concept i can confirm it is working, as I have successfully covered several Xbootclassapth misusing projects.