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

Conflict between jmockit and jacoco #272

Merged
merged 1 commit into from Jan 19, 2015

Conversation

Projects
None yet
4 participants
@marchof
Member

marchof commented Jan 19, 2015

Related to #35 but it seems to still happen.

This project reproduces the issue: https://github.com/henri-tremblay/jacocomockit

When the line in the readme is executed, you will get this exception:
java.lang.UnsupportedOperationException: class redefinition failed: attempted to change the schema (add/remove fields)

jmockit 1.14 and jacoco 0.7.2.201409121644 is used

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Jan 15, 2015

I investigated the issue, and added an explanation and a suggested solution to be applied to JaCoCo, in the corresponding JMockit issue (#125).

For short, the suggested solution is to add the following at the beginning of CoverageTransformer#transform:

if (protectionDomain.getCodeSource().getLocation() == null) return null;

This will prevent any dynamically generated class from being instrumented.

rliesenfeld commented Jan 15, 2015

I investigated the issue, and added an explanation and a suggested solution to be applied to JaCoCo, in the corresponding JMockit issue (#125).

For short, the suggested solution is to add the following at the beginning of CoverageTransformer#transform:

if (protectionDomain.getCodeSource().getLocation() == null) return null;

This will prevent any dynamically generated class from being instrumented.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 16, 2015

Member

@rliesenfeld Thanks for sharing your insights here! Nice to see open source projects contributing to each other.

Do you have some background information about this at hand? I would like to give this a try but really want to also understand it properly. What scares me a bit is that JavaDoc says that ProtectionDomain.getCodeSource() can also return null (but does also not specify in which cases).

Member

marchof commented Jan 16, 2015

@rliesenfeld Thanks for sharing your insights here! Nice to see open source projects contributing to each other.

Do you have some background information about this at hand? I would like to give this a try but really want to also understand it properly. What scares me a bit is that JavaDoc says that ProtectionDomain.getCodeSource() can also return null (but does also not specify in which cases).

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Jan 17, 2015

Yes, the code source can be null as well. Also, the parameter receiving the protection domain itself can be null. So, three null checks are needed in the end. Normally, these cases can only occur for dynamically generated classes, which can't be shown in a coverage report anyway. In JMockit Coverage, I already exclude such cases from instrumentation.

rliesenfeld commented Jan 17, 2015

Yes, the code source can be null as well. Also, the parameter receiving the protection domain itself can be null. So, three null checks are needed in the end. Normally, these cases can only occur for dynamically generated classes, which can't be shown in a coverage report anyway. In JMockit Coverage, I already exclude such cases from instrumentation.

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Jan 17, 2015

To complement, the ProtectionDomain is set on a new class through the ClassLoader#defineClass method (an overload with that parameter), so it really depends on how exactly the class loader is implemented.

rliesenfeld commented Jan 17, 2015

To complement, the ProtectionDomain is set on a new class through the ClassLoader#defineClass method (an overload with that parameter), so it really depends on how exactly the class loader is implemented.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 19, 2015

Member

so it really depends on how exactly the class loader is implemented

JaCoCo is used a lot for integration testing where the code under test is deployed to e.g. J2EE servers or OSGi containers. So we should do at least some smoke tests that these environments properly set codeSource.

Member

marchof commented Jan 19, 2015

so it really depends on how exactly the class loader is implemented

JaCoCo is used a lot for integration testing where the code under test is deployed to e.g. J2EE servers or OSGi containers. So we should do at least some smoke tests that these environments properly set codeSource.

GitHub #272: Exclude dynamically generated classes
Exclude dynamically generated classes from instrumentation for better
interoperability with JMockit.

marchof added a commit that referenced this pull request Jan 19, 2015

Merge pull request #272 from jacoco/issue-272
Conflict between jmockit and jacoco

@marchof marchof merged commit a4654a3 into master Jan 19, 2015

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 9, 2015

Member

This change has caused at least two regressions: #288 and #294
We need to find a way to fix this:

  1. Rollback
  2. Make it optional (off by default)
Member

marchof commented Mar 9, 2015

This change has caused at least two regressions: #288 and #294
We need to find a way to fix this:

  1. Rollback
  2. Make it optional (off by default)
@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 9, 2015

Member

@marchof what about option 3 - updates in upstreams (#294 - Powermock , #288 - something in stack for Android) ? ;)

Member

Godin commented Mar 9, 2015

@marchof what about option 3 - updates in upstreams (#294 - Powermock , #288 - something in stack for Android) ? ;)

@Godin Godin deleted the issue-272 branch Mar 9, 2015

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 9, 2015

Member

If that is a realistic option...

Member

marchof commented Mar 9, 2015

If that is a realistic option...

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 9, 2015

Member

@marchof and what about another option, which is originated in JMockit issue (#125): remove field on our side, so that to avoid root cause of this PR - "class redefinition failed" ?

Member

Godin commented Mar 9, 2015

@marchof and what about another option, which is originated in JMockit issue (#125): remove field on our side, so that to avoid root cause of this PR - "class redefinition failed" ?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 9, 2015

Member

@Godin This would cause a significant performance problem: For every method invocation we would need to access the JaCoCo runtime.

Member

marchof commented Mar 9, 2015

@Godin This would cause a significant performance problem: For every method invocation we would need to access the JaCoCo runtime.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 9, 2015

Member

@marchof As stated in referenced ticket - they don't observe such problem with jmockit-coverage. Would be nice to have a benchmark as a proof of such problem. Also I'm wondering how this is done in JCov.

Member

Godin commented Mar 9, 2015

@marchof As stated in referenced ticket - they don't observe such problem with jmockit-coverage. Would be nice to have a benchmark as a proof of such problem. Also I'm wondering how this is done in JCov.

@henri-tremblay

This comment has been minimized.

Show comment
Hide comment
@henri-tremblay

henri-tremblay Mar 9, 2015

I'm a bit lost between the issue. However, provided with a before / after
code. I'm willing to perform the benchmark

On 9 March 2015 at 13:17, Evgeny Mandrikov notifications@github.com wrote:

@marchof https://github.com/marchof As stated in referenced ticket -
they don't observe such problem with jmockit-coverage. Would be nice to
have a benchmark as a proof of such problem. Also I'm wondering how this is
done in JCov.


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

henri-tremblay commented Mar 9, 2015

I'm a bit lost between the issue. However, provided with a before / after
code. I'm willing to perform the benchmark

On 9 March 2015 at 13:17, Evgeny Mandrikov notifications@github.com wrote:

@marchof https://github.com/marchof As stated in referenced ticket -
they don't observe such problem with jmockit-coverage. Would be nice to
have a benchmark as a proof of such problem. Also I'm wondering how this is
done in JCov.


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

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 9, 2015

Member

@henri-tremblay currently there is no before/after , because we were discussing possible options.

Member

Godin commented Mar 9, 2015

@henri-tremblay currently there is no before/after , because we were discussing possible options.

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Mar 9, 2015

I believe this fix should stay as it is. See discussion at #294

A performance benchmark comparing JaCoCo to JMockit Coverage would be interesting; I may create one myself, eventually. Just for awareness, one significant performance issue I noticed in JaCoCo is that, by default (ie, when users don't specify the classes to be included or excluded, which seems to be the common case), it will instrument all classes from third-party libraries; obviously, users generally don't want to measure coverage for such classes, resulting in a lot of unnecessary instrumentation. JMockit Coverage avoids this issue by only instrumenting classes that don't come from a jar file, except for classes explicitly selected for coverage; this may result in a significant performance gain for typical usage scenarios.

rliesenfeld commented Mar 9, 2015

I believe this fix should stay as it is. See discussion at #294

A performance benchmark comparing JaCoCo to JMockit Coverage would be interesting; I may create one myself, eventually. Just for awareness, one significant performance issue I noticed in JaCoCo is that, by default (ie, when users don't specify the classes to be included or excluded, which seems to be the common case), it will instrument all classes from third-party libraries; obviously, users generally don't want to measure coverage for such classes, resulting in a lot of unnecessary instrumentation. JMockit Coverage avoids this issue by only instrumenting classes that don't come from a jar file, except for classes explicitly selected for coverage; this may result in a significant performance gain for typical usage scenarios.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 9, 2015

Member

@Godin I added a separate PR #296 for instrumentation without adding members.

Member

marchof commented Mar 9, 2015

@Godin I added a separate PR #296 for instrumentation without adding members.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 31, 2016

This will prevent any dynamically generated class from being instrumented.

We use apache-tapestry-ioc and this change just moved all our tapestry service coverage to 0%
This change prevents any code in actual classes called through dynamically generated classes from being considered.
You should skip dynamically generated classes from being processed, but you still need to dig further because there can be actual code coverage further down the stack trace.
We are currently stuck with using jacoco 0.7.2.201409121644 because of this "feature"

ghost commented Mar 31, 2016

This will prevent any dynamically generated class from being instrumented.

We use apache-tapestry-ioc and this change just moved all our tapestry service coverage to 0%
This change prevents any code in actual classes called through dynamically generated classes from being considered.
You should skip dynamically generated classes from being processed, but you still need to dig further because there can be actual code coverage further down the stack trace.
We are currently stuck with using jacoco 0.7.2.201409121644 because of this "feature"

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 31, 2016

Member

@pedrocborges2 JaCoCo 0.7.6 has configuration option "inclnolocationclasses" ( #288 ).

Member

Godin commented Mar 31, 2016

@pedrocborges2 JaCoCo 0.7.6 has configuration option "inclnolocationclasses" ( #288 ).

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 31, 2016

Thank you @Godin I somehow missed that.

ghost commented Mar 31, 2016

Thank you @Godin I somehow missed that.

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