-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add filter for instructions inlined by Kotlin compiler #764
Conversation
9776f8f
to
04523d4
Compare
@marchof could you please do first review pass? |
org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/KotlinInlineFilter.java
Outdated
Show resolved
Hide resolved
org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/KotlinInlineFilter.java
Show resolved
Hide resolved
org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/KotlinInlineFilter.java
Outdated
Show resolved
Hide resolved
a593487
to
3710462
Compare
3710462
to
3a445bd
Compare
@jwalter thanks a lot for testing and this valuable feedback just in time to keep our master free from bugs 👍 our small team really appreciates this ❤️ Don't know yet how to reproduce this, but apparently |
Not sure it helps, but the class triggering looks like this (lines 8-20. 1-7 are package declaration and imports):
|
@jwalter thank you for the code - it gave an idea about reproducer, which is actually quite simple: compilation of
results in
Will work on a fix in context of this ticket. However this is also quite interesting in context of a #654 (comment) (CC @goodwinnk @yole): content of |
And here is another example:
that produces
From which to me seems that we can change algorithm on following:
Any feedback/thoughts about this new algorithm are welcome. |
I was wrong about current validation test 😞
|
excluding `LineInfo`s whose `LineFileID` corresponds to `SourceFile` attribute
+and `InputStartLine` equal to `OutputStartLine` |
@jwalter could you please retest? |
@Godin Works great! (My coverage went from 40% to 90%) |
@jwalter thanks a lot for testing! 👍 ❤️ Please excuse me for pedantism, but as a remark - when talking about metrics it is IMO very important to always specify metric, i.e. percentage of what? "instructions"? 😉 Nevertheless, while #763 initially is about mismatch between numbers of lines in XML report and source files, another aspect - is indeed a fix of metrics (instructions, lines, branches, complexity), which before this change were including inlined code that potentially comes from other libraries and therefore doesn't require tests on a side of consumers of library. Thanks again for spotting bug and testing! ❤️ Hope there won't be other obstacles and we'll publish this soon. |
Fixes #763
After this change for the same example as in #763 :