Provide better coverage data in case of implicit exceptions

[marchof]

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]

For comparison

this PR:

PR #261:

master:

[marchof]

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

[mackerl]

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]

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]

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

Here is a build for this PR: http://download.eclipselab.org/jacoco/preview/jacoco-0.7.5.201505190939.zip

[mackerl]

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

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

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

[marchof]

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

[marchof]

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]

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

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

[Godin]

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

[Godin]

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

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

[marchof]

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

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]

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

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

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

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

[marchof]

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]

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

[marchof]

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

[Godin]

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

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

[Godin]

@marchof now I see two scenarios, while originally I had in mind only first one:

1. in case if all tests touch same code and endup in exception thrown from method (or more realistic - execution of single test from IDE), then for me seems that there is no need for additional probe - fact of exception can be derived by looking on subsequent probe
2. in case of multiple tests, where some touch exception and other not, then indeed I see what you mean by "exception semantic" - even if line fully covered might be interesting to know that some tests were stopped in the middle due to exception

[Godin]

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

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

[Godin]

typo: "id" -> "if"

[marchof]

Done.

[marchof]

@Godin Anything that prevents us from merging this?

[Godin]

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

[scottcarey]

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().

[edisonh]

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

[C-Otto]

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