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

Provide better coverage data in case of implicit exceptions #310

Merged
merged 1 commit into from May 22, 2015

Conversation

Projects
None yet
7 participants
@marchof
Member

marchof commented May 18, 2015

This is an alternative approach to the PR proposed in #261. The objective is to improve covered result in case implicit exceptions are thrown by method invocations. Instead of adding a extra probe for every method invocation this PR adds probes only between source lines -- and only if the subsequent line contains at least one method invocation.

This reduces the number of extra probes needed and at the same time provides better results in case of implicit exceptions in multi-line blocks.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 18, 2015

Member

For comparison

this PR:
pr-310

PR #261:
pr-261

master:
master

Member

Godin commented May 18, 2015

For comparison

this PR:
pr-310

PR #261:
pr-261

master:
master

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

@Godin Nice, thx! Another useful comparison would be the total number of probes and the execution duration for a reasonable test suite.

Member

marchof commented May 19, 2015

@Godin Nice, thx! Another useful comparison would be the total number of probes and the execution duration for a reasonable test suite.

@mackerl

This comment has been minimized.

Show comment
Hide comment
@mackerl

mackerl May 19, 2015

I plan a testrun with this pull request this weekend. Afterwards I will give you a comparison about runtime and coverage results between the two PRs

mackerl commented May 19, 2015

I plan a testrun with this pull request this weekend. Afterwards I will give you a comparison about runtime and coverage results between the two PRs

@mackerl

This comment has been minimized.

Show comment
Hide comment
@mackerl

mackerl May 19, 2015

Testsuite with 6000 JUnit Tests, 3 Million LoC under Test, 48% Coverage
without Coverage: 12 hours
with Coverage from Master without PR #261 29 hours
with Coverage from Master with PR #261 30 hours
with Coverage from Master with PR #310 coming soon

mackerl commented May 19, 2015

Testsuite with 6000 JUnit Tests, 3 Million LoC under Test, 48% Coverage
without Coverage: 12 hours
with Coverage from Master without PR #261 29 hours
with Coverage from Master with PR #261 30 hours
with Coverage from Master with PR #310 coming soon

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

@mackerl Wow, 12h for 6000 Unit-Tests? Are this actually unit tests? Or do you fork a VM for each test case? This would also explain why the runtime with coverage more than doubles. Just a comparison: JaCoCo runs 1000 Tests with coverage in 80sec:

http://www.eclemma.org/jacoco/trunk/test/index.html

Anyways, many thanks for your ongoing feedback, very helpful!

Member

marchof commented May 19, 2015

@mackerl Wow, 12h for 6000 Unit-Tests? Are this actually unit tests? Or do you fork a VM for each test case? This would also explain why the runtime with coverage more than doubles. Just a comparison: JaCoCo runs 1000 Tests with coverage in 80sec:

http://www.eclemma.org/jacoco/trunk/test/index.html

Anyways, many thanks for your ongoing feedback, very helpful!

@marchof

This comment has been minimized.

Show comment
Hide comment
Member

marchof commented May 19, 2015

@mackerl

This comment has been minimized.

Show comment
Hide comment
@mackerl

mackerl May 19, 2015

@marchof You are welcome. A few hundred of the tests are more on the API Layer. They have a runtime from a few minutes up to one hour. We also do not mock everything - there are actually around 1000 tests involving a real Oracle or SqlServer Database, where a specific datastage is set up during the bootstrap of the Unit Test Framework. Those tests typically run up to a few minutes. Nevertheless the biggest part of our testsuite are real unit tests with a runtime of around a few milli seconds.
Thanks for the build .... would it be an easy task for you to provide an eclemma update site, where this change in jacoco is included? Otherwise I can also do it by myself

mackerl commented May 19, 2015

@marchof You are welcome. A few hundred of the tests are more on the API Layer. They have a runtime from a few minutes up to one hour. We also do not mock everything - there are actually around 1000 tests involving a real Oracle or SqlServer Database, where a specific datastage is set up during the bootstrap of the Unit Test Framework. Those tests typically run up to a few minutes. Nevertheless the biggest part of our testsuite are real unit tests with a runtime of around a few milli seconds.
Thanks for the build .... would it be an easy task for you to provide an eclemma update site, where this change in jacoco is included? Otherwise I can also do it by myself

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

@mackerl Sorry, my local build infrastructure currently only allows to build EclEmma from Maven repo. Could work on this but not on short notice.

Member

marchof commented May 19, 2015

@mackerl Sorry, my local build infrastructure currently only allows to build EclEmma from Maven repo. Could work on this but not on short notice.

@mackerl

This comment has been minimized.

Show comment
Hide comment
@mackerl

mackerl May 19, 2015

@marchof no problem ... i will hack maven myself again then :)

mackerl commented May 19, 2015

@marchof no problem ... i will hack maven myself again then :)

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

@mackerl What counter does your coverage percentage refer to? Would be nice to know the percentages for onstructions, branches and lines separately.

Member

marchof commented May 19, 2015

@mackerl What counter does your coverage percentage refer to? Would be nice to know the percentages for onstructions, branches and lines separately.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

I will create a comparison for commons-collections tests tonight, camparing the following figures:

  • execution time
  • covered and total instructions
  • covered and total branches
  • covered and total lines
  • total number of probes
  • exec file size
Member

marchof commented May 19, 2015

I will create a comparison for commons-collections tests tonight, camparing the following figures:

  • execution time
  • covered and total instructions
  • covered and total branches
  • covered and total lines
  • total number of probes
  • exec file size
@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

@marchof I'm wondering if possible to update your PR to display lines with exceptions as partially covered as it was done in PR #261 ? IMO this makes report in such cases more readable.

Member

Godin commented May 19, 2015

@marchof I'm wondering if possible to update your PR to display lines with exceptions as partially covered as it was done in PR #261 ? IMO this makes report in such cases more readable.

@andrioli

This comment has been minimized.

Show comment
Hide comment
@andrioli

andrioli May 19, 2015

Contributor

+1 for @Godin proposal. IMO the partially covered lines make the report more "understandable".

Contributor

andrioli commented May 19, 2015

+1 for @Godin proposal. IMO the partially covered lines make the report more "understandable".

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

Indeed - I meant "understandable", when wrote "readable" 😉 Thx for correction @andrioli

Member

Godin commented May 19, 2015

Indeed - I meant "understandable", when wrote "readable" 😉 Thx for correction @andrioli

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

@marchof out of InstrumentationBenchmark - comparison of "cold" performance of instrumentation of jar, supposed that offline instrumentation not much different from online ( 10 forks, 1 iteration, no warmup, JDK7u80 ) :

elasticsearch = org.elasticsearch:elasticsearch:1.1.2 , 13M
guava = com.google.guava:guava:18.0 , 2.2M
rt = rt.jar of used JVM , 60M

master:

  elasticsearch: 4372.632 ± 142.458 ms/op
          guava: 1763.230 ±  33.403 ms/op
             rt: 8443.019 ± 257.801 ms/op

this PR:

  elasticsearch: 4373.558 ± 106.255 ms/op
          guava: 1815.460 ±  77.315 ms/op
             rt: 8574.634 ± 435.716 ms/op

PR #261:

  elasticsearch: 4366.380 ± 167.617 ms/op
          guava: 1822.976 ± 141.603 ms/op
             rt: 8502.746 ± 291.181 ms/op

Note that benchmark can be improved to exclude time of jar reading, but IMO it already shows that we can forget about impact of changes on instrumentation time for both PRs.

Member

Godin commented May 19, 2015

@marchof out of InstrumentationBenchmark - comparison of "cold" performance of instrumentation of jar, supposed that offline instrumentation not much different from online ( 10 forks, 1 iteration, no warmup, JDK7u80 ) :

elasticsearch = org.elasticsearch:elasticsearch:1.1.2 , 13M
guava = com.google.guava:guava:18.0 , 2.2M
rt = rt.jar of used JVM , 60M

master:

  elasticsearch: 4372.632 ± 142.458 ms/op
          guava: 1763.230 ±  33.403 ms/op
             rt: 8443.019 ± 257.801 ms/op

this PR:

  elasticsearch: 4373.558 ± 106.255 ms/op
          guava: 1815.460 ±  77.315 ms/op
             rt: 8574.634 ± 435.716 ms/op

PR #261:

  elasticsearch: 4366.380 ± 167.617 ms/op
          guava: 1822.976 ± 141.603 ms/op
             rt: 8502.746 ± 291.181 ms/op

Note that benchmark can be improved to exclude time of jar reading, but IMO it already shows that we can forget about impact of changes on instrumentation time for both PRs.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

@marchof and btw I believe that file size computable from number of probes, so why you plan to track it?

Member

Godin commented May 19, 2015

@marchof and btw I believe that file size computable from number of probes, so why you plan to track it?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

@Godin exec files contains meta information (class name, id) and probes. As probes are stored as bits they make only 50%. But doesn't actually matter, I'll post my results in a few minutes.

Member

marchof commented May 19, 2015

@Godin exec files contains meta information (class name, id) and probes. As probes are stored as bits they make only 50%. But doesn't actually matter, I'll post my results in a few minutes.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

Here are the results for commons-collections (14,594 test cases):

Branch Runtime Exec File Size Probes Instructions Branches Lines
Run 5.919s
Master 7.222s 58,182 15,988 109,092 / 130,209 6,005 / 8,364 25,804 / 30,841
PR 310 7.421s 59,944 29,851 109,455 / 130,209 6,004 / 8,364 25,869 / 30,841
PR 261 7.571s 62,570 50,086 111,475 / 130,209 6,010 / 8,364 26,460 / 30,841

While we see a significant increase in probes between the different approaches, runtime seems not be really effected. Also memory overhead should be minimal as we need 1 byte per probe. As a conclusion probes are pretty cheap and we can actually heavily use them!

Member

marchof commented May 19, 2015

Here are the results for commons-collections (14,594 test cases):

Branch Runtime Exec File Size Probes Instructions Branches Lines
Run 5.919s
Master 7.222s 58,182 15,988 109,092 / 130,209 6,005 / 8,364 25,804 / 30,841
PR 310 7.421s 59,944 29,851 109,455 / 130,209 6,004 / 8,364 25,869 / 30,841
PR 261 7.571s 62,570 50,086 111,475 / 130,209 6,010 / 8,364 26,460 / 30,841

While we see a significant increase in probes between the different approaches, runtime seems not be really effected. Also memory overhead should be minimal as we need 1 byte per probe. As a conclusion probes are pretty cheap and we can actually heavily use them!

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

@marchof SonarSource C++ Plugin, modular project but all classes included in instrumentation and so in dump files , 48K of source code in 1K of classes not counting tests , 2400 tests (not all are really "unit" - a lot of "medium") and 92.7% coverage as reported by SonarQube with JaCoCo 0.7.4 , single shot, wall clock time, first measurement - "mvn test", subsequent - "mvn jacoco:prepare-agent test" :

without coverage = 1m18.3s
master = 1m36.9s
this PR = 1m36.8s
PR #261 = 1m37.1s

We don't have aggregated JaCoCo reports, so can't quickly provide all other numbers, but can work on this if needed.

Taking into account run-to-run variance I consider that all 3 branches actually demonstrate same time, so changes are insignificant.

Member

Godin commented May 19, 2015

@marchof SonarSource C++ Plugin, modular project but all classes included in instrumentation and so in dump files , 48K of source code in 1K of classes not counting tests , 2400 tests (not all are really "unit" - a lot of "medium") and 92.7% coverage as reported by SonarQube with JaCoCo 0.7.4 , single shot, wall clock time, first measurement - "mvn test", subsequent - "mvn jacoco:prepare-agent test" :

without coverage = 1m18.3s
master = 1m36.9s
this PR = 1m36.8s
PR #261 = 1m37.1s

We don't have aggregated JaCoCo reports, so can't quickly provide all other numbers, but can work on this if needed.

Taking into account run-to-run variance I consider that all 3 branches actually demonstrate same time, so changes are insignificant.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

@marchof even, if addition of probes has insignificant impact on performance, we anyway should be careful at least due to limits on size of methods 😈 and what about impact on size of class files on disk and in memory, and impact on size of compiled code in memory? 😆

Member

Godin commented May 19, 2015

@marchof even, if addition of probes has insignificant impact on performance, we anyway should be careful at least due to limits on size of methods 😈 and what about impact on size of class files on disk and in memory, and impact on size of compiled code in memory? 😆

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

@marchof did you noticed difference in covered branches and lines in results for commons-collections? I just wanted to make a note on this measurement - don't exclude possible existence of run-to-run variance, I fighted a lot to get rid of it in project mentioned before.

Member

Godin commented May 19, 2015

@marchof did you noticed difference in covered branches and lines in results for commons-collections? I just wanted to make a note on this measurement - don't exclude possible existence of run-to-run variance, I fighted a lot to get rid of it in project mentioned before.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

@Godin yes, there are differences, see my table above. The columns Instructions, Branches and Lines show covered vs. total items each.

Member

marchof commented May 19, 2015

@Godin yes, there are differences, see my table above. The columns Instructions, Branches and Lines show covered vs. total items each.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

Regarding coverage status of methods with exceptions: If we spent a probe per method invocation we can even mark those lines as "green".

BTW, there are a couple of other implicit exceptions (NPE, IndexOutOfBounds etc.) which will still not result in coverage.

Member

marchof commented May 19, 2015

Regarding coverage status of methods with exceptions: If we spent a probe per method invocation we can even mark those lines as "green".

BTW, there are a couple of other implicit exceptions (NPE, IndexOutOfBounds etc.) which will still not result in coverage.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

@marchof my point was that maybe you'll see difference even between multiple runs of exactly the same JaCoCo build

Member

Godin commented May 19, 2015

@marchof my point was that maybe you'll see difference even between multiple runs of exactly the same JaCoCo build

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

@Godin I just tried with master: The figures seem to be reproducible.

Member

marchof commented May 19, 2015

@Godin I just tried with master: The figures seem to be reproducible.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

@marchof not sure that I understand your point about "green" lines. Original proposal was to show lines 12, 21, 27, 36, 44, 48 as partially covered (see screenshots). You propose to show them as green aka fully covered? If so, then understandably is the same as in case of red aka not-covered - let's take line 12 as example with slightly changed names of methods:

a(); // green
b(); // if green or red, then where exception happened? in method "b" or in method "c"?
     // and in case of yellow it is clear that exception happened in method "b".
c(); // red
Member

Godin commented May 19, 2015

@marchof not sure that I understand your point about "green" lines. Original proposal was to show lines 12, 21, 27, 36, 44, 48 as partially covered (see screenshots). You propose to show them as green aka fully covered? If so, then understandably is the same as in case of red aka not-covered - let's take line 12 as example with slightly changed names of methods:

a(); // green
b(); // if green or red, then where exception happened? in method "b" or in method "c"?
     // and in case of yellow it is clear that exception happened in method "b".
c(); // red
@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

@marchof ok, so then I suppose that figures are different exactly because of difference of branches on not-covered and partially-covered.

Member

Godin commented May 19, 2015

@marchof ok, so then I suppose that figures are different exactly because of difference of branches on not-covered and partially-covered.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 19, 2015

Member

@marchof and so then current behaviour correct and logical, whereas showing of line as green aka fully-covered IMO will be not really correct, thus probably would be better to go ahead with this PR as it is now and to consider introduction of "exception semantic" as separate enhancement additional to this.

Member

Godin commented May 19, 2015

@marchof and so then current behaviour correct and logical, whereas showing of line as green aka fully-covered IMO will be not really correct, thus probably would be better to go ahead with this PR as it is now and to consider introduction of "exception semantic" as separate enhancement additional to this.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 19, 2015

Member

@Godin Fine to me. May I ask you to review the code changes?

Member

marchof commented May 19, 2015

@Godin Fine to me. May I ask you to review the code changes?

@Godin Godin self-assigned this May 19, 2015

testNeedsProbe(false, false, true, false);
testNeedsProbe(true, false, true, true);
testNeedsProbe(false, true, true, true);
testNeedsProbe(true, true, true, true);

This comment has been minimized.

@Godin

Godin May 20, 2015

Member

not sure that such style of tests is readable, but that's minor

@Godin

Godin May 20, 2015

Member

not sure that such style of tests is readable, but that's minor

This comment has been minimized.

@marchof

marchof May 20, 2015

Member

Any other proposal? Alternatively I can create scenario tests for this (as already used in this test class).

@marchof

marchof May 20, 2015

Member

Any other proposal? Alternatively I can create scenario tests for this (as already used in this test class).

This comment has been minimized.

@Godin

Godin May 20, 2015

Member

@marchof fine to keep it as is.

@Godin

Godin May 20, 2015

Member

@marchof fine to keep it as is.

Show outdated Hide outdated org.jacoco.doc/docroot/doc/faq.html Outdated

@Godin Godin assigned marchof and unassigned Godin May 20, 2015

@marchof marchof added this to the 0.7.5 milestone May 20, 2015

@marchof marchof changed the title from Provide better coverage data in case of implicit excptions to Provide better coverage data in case of implicit exceptions May 21, 2015

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 21, 2015

Member

@Godin Anything that prevents us from merging this?

Member

marchof commented May 21, 2015

@Godin Anything that prevents us from merging this?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 21, 2015

Member

@marchof nothing! you can go ahead or you wanna me to do squash/merge?

Member

Godin commented May 21, 2015

@marchof nothing! you can go ahead or you wanna me to do squash/merge?

Improve coverage data for code with implicit exception.
Add additional probe before every line with at least one method
invocation.

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

Merge pull request #310 from jacoco/issue-310
Provide better coverage data in case of implicit exceptions.

@marchof marchof merged commit 994c1d7 into master May 22, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@marchof marchof deleted the issue-310 branch May 22, 2015

rianniello added a commit to ausaccessfed/aaf-shib-ext that referenced this pull request Jun 10, 2015

Upgraded jacocoa
This bug jacoco/jacoco#310 caused coverage to
be reported lower than it is.
@scottcarey

This comment has been minimized.

Show comment
Hide comment
@scottcarey

scottcarey Jun 11, 2015

The new coverage form does not make sense to me. I understand why it is happening, but if you call

ex();

which always throws.

Then it is always thus 'red' at the call site, yet the inside of its method is green! This is inconsistent. How could you have coverage inside ex() if you never called it? That is what is implied, to me, by ex() being red. Execution never got there, and stopped the line before.

Hopefully the new 'exception semantic' fixes this, because yellow made way more sense to me here -- we entered, but did not leave, ex().

scottcarey commented Jun 11, 2015

The new coverage form does not make sense to me. I understand why it is happening, but if you call

ex();

which always throws.

Then it is always thus 'red' at the call site, yet the inside of its method is green! This is inconsistent. How could you have coverage inside ex() if you never called it? That is what is implied, to me, by ex() being red. Execution never got there, and stopped the line before.

Hopefully the new 'exception semantic' fixes this, because yellow made way more sense to me here -- we entered, but did not leave, ex().

C-Otto added a commit to C-Otto/bitcoinj that referenced this pull request Jul 4, 2015

upgrade jacoco version
0.7.5 introduces a new binary format, see:
jacoco/jacoco#310
jacoco/jacoco#261

@C-Otto C-Otto referenced this pull request Jul 4, 2015

Closed

upgrade jacoco version #1012

C-Otto added a commit to C-Otto/bitcoinj that referenced this pull request Jul 4, 2015

Upgrade jacoco version.
Jacoco 0.7.5 introduces a new binary format, see:
jacoco/jacoco#310
jacoco/jacoco#261

schildbach added a commit to bitcoinj/bitcoinj that referenced this pull request Jul 4, 2015

Upgrade jacoco version.
Jacoco 0.7.5 introduces a new binary format, see:
jacoco/jacoco#310
jacoco/jacoco#261
@edisonh

This comment has been minimized.

Show comment
Hide comment
@edisonh

edisonh Feb 23, 2016

+1 for @scottcarey proposal.
Before introduction of "exception semantic", yellow is more semantic.

edisonh commented Feb 23, 2016

+1 for @scottcarey proposal.
Before introduction of "exception semantic", yellow is more semantic.

@C-Otto

This comment has been minimized.

Show comment
Hide comment
@C-Otto

C-Otto Jul 20, 2016

+1 from me. I still prefer my original PR #261 for what it's worth.

C-Otto commented Jul 20, 2016

+1 from me. I still prefer my original PR #261 for what it's worth.

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