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

Include possibility of covering of boot-classpath #49

Merged
merged 2 commits into from May 22, 2014

Conversation

Projects
None yet
3 participants
@marchof
Member

marchof commented May 19, 2014

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.

@judovana

This comment has been minimized.

Show comment
Hide comment
@judovana

judovana Dec 11, 2012

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:
http://icedtea.classpath.org/hg/icedtea-web/file/1fe2a4f7981f/Makefile.am#l71 - 73.

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!

judovana commented Dec 11, 2012

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:
http://icedtea.classpath.org/hg/icedtea-web/file/1fe2a4f7981f/Makefile.am#l71 - 73.

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!

@judovana

This comment has been minimized.

Show comment
Hide comment
@judovana

judovana May 13, 2014

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 commented May 13, 2014

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?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 13, 2014

Member

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

Member

marchof commented May 13, 2014

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

@judovana

This comment has been minimized.

Show comment
Hide comment
@judovana

judovana May 13, 2014

Thank you very much! If it will be awesome.

judovana commented May 13, 2014

Thank you very much! If it will be awesome.

GitHub #49: New agent option includebootstrapclasses
New configuration option for the JaCoCo agent includebootstrapclasses to
also instrument classes from the bootstrap class loader.
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2014

Member

@judovana I attached a pull request to this issue. Please feel free to comment on it.

  • The attribute is called includebootstrapclasses
  • various tests addesd
  • documentation added
Member

marchof commented May 19, 2014

@judovana I attached a pull request to this issue. Please feel free to comment on it.

  • The attribute is called includebootstrapclasses
  • various tests addesd
  • documentation added
@judovana

This comment has been minimized.

Show comment
Hide comment
@judovana

judovana May 20, 2014

Hi!

I have checked a changeset and it looks ok. (I have no yet tried an build) 
I'm looking forward to see it working!

Just for curiosity - when is planned next release?

Thank yu very much!

Mgr. Jiri Vanek
judovana@email.cz

---------- Původní zpráva ----------
Od: Marc R. Hoffmann notifications@github.com
Komu: jacoco/jacoco jacoco@noreply.github.com
Datum: 19. 5. 2014 21:47:06
Předmět: Re: [jacoco] Include possibility of covering of boot-classpath (#
49)

"

@judovana(https://github.com/judovana) I attached a pull request to this
issue. Please feel free to comment on it.

  • The attribute is called includebootstrapclasses
  • various tests addesd
  • documentation added


Reply to this email directly or view it on GitHub
(#49 (comment)).

"

judovana commented May 20, 2014

Hi!

I have checked a changeset and it looks ok. (I have no yet tried an build) 
I'm looking forward to see it working!

Just for curiosity - when is planned next release?

Thank yu very much!

Mgr. Jiri Vanek
judovana@email.cz

---------- Původní zpráva ----------
Od: Marc R. Hoffmann notifications@github.com
Komu: jacoco/jacoco jacoco@noreply.github.com
Datum: 19. 5. 2014 21:47:06
Předmět: Re: [jacoco] Include possibility of covering of boot-classpath (#
49)

"

@judovana(https://github.com/judovana) I attached a pull request to this
issue. Please feel free to comment on it.

  • The attribute is called includebootstrapclasses
  • various tests addesd
  • documentation added


Reply to this email directly or view it on GitHub
(#49 (comment)).

"

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 20, 2014

Member

@marchof what about addition of integration test for Maven Plugin to cover newly introduced property?

Member

Godin commented May 20, 2014

@marchof what about addition of integration test for Maven Plugin to cover newly introduced property?

@judovana

This comment has been minimized.

Show comment
Hide comment
@judovana

judovana May 20, 2014

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

judovana commented May 20, 2014

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

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 21, 2014

Member

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

Member

marchof commented May 21, 2014

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

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 22, 2014

Member

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

Member

Godin commented May 22, 2014

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

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 22, 2014

Member

@Godin So good to merge?

Member

marchof commented May 22, 2014

@Godin So good to merge?

marchof added a commit that referenced this pull request May 22, 2014

Merge pull request #49 from jacoco/issue-49
Include possibility of covering of boot-classpath

@marchof marchof merged commit e7528a5 into master May 22, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@marchof marchof deleted the issue-49 branch May 22, 2014

@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017

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