-
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
Exclude methods annotated with @lombok.Generated #513
Conversation
Based in initial contribution by Rüdiger zu Dohna.
* http://www.eclipse.org/legal/epl-v10.html | ||
* | ||
* Contributors: | ||
* Rüdiger zu Dohna - initial API and implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof even if initial analysis wasn't done by you, all code of this PR was entirely written by you, so I believe this line should be changed - anyway there will be credits in changelog
@Test | ||
public void testLombokGeneratedAnnotation() { | ||
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, | ||
Opcodes.ACC_SYNTHETIC, "lambda$1", "()V", null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof synthetic access flag and name of lambda are unrelated to this test, so I believe that for clarity should be changed. Name can be changed for example on hashCode
as one of examples of methods that Lombok generates. Can be changed for the all test cases.
} | ||
|
||
private boolean hasLombokGeneratedAnnotation(final MethodNode methodNode) { | ||
final List<AnnotationNode> list = methodNode.invisibleAnnotations; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof I'd prefer to name local variable invisibleAnnotations
or even better runtimeInvisibleAnnotations
instead of just list
@@ -28,6 +28,9 @@ | |||
<li>Exclude from a report a part of bytecode that compiler generates for a | |||
synchronized statement | |||
(GitHub <a href="https://github.com/jacoco/jacoco/issues/501">#501</a>).</li> | |||
<li>Exclude methods generated by Lombok. Initial analysis and contribution by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof I'd prefer to be more explicit here and state that
- they are excluded from report - to avoid confusion with exclusion from instrumentation
- only methods annotated with
lombok.Generated
are excluded - because annotation can be turned off
For consistency with previous changelog entries - we don't put dot before opening parenthesis.
So something like:
Exclude from a report methods annotated by <code>lombok.Generated</code>,
initial analysis contributed by Rüdiger zu Dohna (GitHub ...).
import org.objectweb.asm.tree.MethodNode; | ||
|
||
/** | ||
* Filter for methods generated by Lombok which are annotated with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof for consistency with existing filters, I'd prefer to change on
Filters methods annotated with <code>@lombok.Generated</code>.
@Test | ||
public void testOtherAnnotation() { | ||
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, | ||
Opcodes.ACC_SYNTHETIC, "name", "()V", null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof synthetic access flag is unrelated to this test, so I'd prefer to remove it
m.visitInsn(Opcodes.RETURN); | ||
|
||
AnnotationNode a = new AnnotationNode("Llombok/Generated;"); | ||
m.invisibleAnnotations = Arrays.asList(a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on initial contribution by Rüdiger zu Dohna.
@Godin thx for the review! Hopefully fixed all issues. |
Based on initial contribution by Rüdiger zu Dohna.
End-to-end test - example.zip Without this change using JaCoCo 0.7.9 : After this change : |
Hi, nice one. I'd like to try this out. I let lombok generate "lombok.Generated" and use the jacoco-agent with gradle. I tried the snapshot from 20170404 (-17) and it does not seem to be in there. Or do I have to configure jacoco somehow? |
@MichaelZett I just double checked the latest snaphot (from 20170404 on) has this feature. Are you sure you're using the latest version of Lombok? The |
Works fine. Big thanks! When I first looked at the code, I thought it might not work when there are other annotations next to @lombok.Getter(onMethod_ = { @javax.xml.bind.annotation.XmlAttribute(name = "bar") })
private String foo; But it does work ;-) Is it foreseeable when this may get released? |
@marchof yes, Lombok annotation is generated. I checked with delombok. Do I have to configure the filters to use? Does anyone has checked it with gradle (3.4.1)? Gradle uses the jacoco agent lib. |
@MichaelZett here is an example with Gradle that works just perfectly - example.zip |
please be patient and give us some time to first finish some other filters. Thank you for your understanding. |
Am I wrong to assume that this may take quite a while, as most of those filters are much harder to build than I don't like the alternative to internally release a fork. |
Filters for |
@Godin Thanks for the zip. When I take this and run gradlew clean build, it produces the attached test.exec. When I look at this in intellij or sonarqube it shows still 4 missed lines. What could be the problem? |
@MichaelZett filtering is performed at a time of report generation (creation of html, xml, etc), not at a time of collection of execution information (creation of exec file). So that tools that read execution data directly instead of reading of xml (which is a kind of mistake on their side to rely on purely internal intermediate format, but what's done is done) and create their own report (such as SonarQube, Jenkins, etc) will need to update their dependency on JaCoCo once it will be released in order to get filtering for reports. We will notify explicitly downstream projects (in particular all mentioned above) about this when our release will be done. So once again - please be patient. Thank you for your understanding. |
@MichaelZett also should be mentioned that reports produced by JaCoCo Gradle Plugin (in |
@Godin I see, thanks. Will be waiting. |
@Godin : I'll be patient... but I feel a little like a child before christmas ;-) |
I have successfully used the snapshot build to exclude Lombok generated code. The generated report works nicely now, thanks for this update. Just curious though: on individual maven projects it works fine, but I have trouble generating an aggregated report for a multi-module Maven project. The reports in each module look fine, but the aggregated report is empty. Has anyone else had this issue? Maybe its a configuration fault on my side, this is what I added to my root pom:
|
@tenleo Thanks for your feedback! Aggregate reports work differently. Please see documentation. You need to specify a separate module which defined the content of the report. As this is a completely different issue please use our mailing list for further questions. |
Hi guys, I'm curious, did anyone make it work with SonarQube? Is it just me? I created a small github project to ease reproduction of the issue: https://github.com/Sir4ur0n/example-jacoco-lombok-sonarqube Here's the JaCoCo report: Any help or pointer on what's wrong would be greatly appreciated :( |
@sir4ur0n please read carefully comments in this thread prior to yours, in particular - #513 (comment) |
@Godin Thanks, I had completely missed this comment :'( |
Do we have an ETA on when 0.7.10 will be released? I couldn't find a release schedule anywhere. Asking specifically for this fix. |
@novaterata please read already existing answers on such question: |
@Godin Thank you, I didn't know where to even look |
Is the filter out of lombok supported already in a released version of JaCoCo?Thanks |
No, it's coming in 0.7.10 You can read the release notes to see when issues
are released
…On Mon, Jul 3, 2017, 8:59 AM Elena6789 ***@***.***> wrote:
Is the filter out of lombok supported already in a released version of
JaCoCo?Thanks
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#513 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABlGHeO_HHrJK9GYoRYVYchOgEYFfkQPks5sKOWbgaJpZM4MxMmI>
.
|
I tried using the latest snapshot, but the report generated in target/site includes the equals(..) method. Has anything changed? Are there specific configurations that one has to enable? |
@Glamdring please read this thread carefully from the beginning - in particular there is full end-to-end example with all the required configuration on Lombok side in #513 (comment) |
@Godin apologies for overlooking that, thanks. |
Guys, I am waiting for about 4 months for this feature. Can you please give a DL when will this feature be released ? |
@miualinionut: I guess it's done when it's done. That's the way open source projects work when they are driven by people spending their spare time. @Godin: I'd rather request an intermediate release of the things that already work instead of waiting until everything is complete. Would that be possible? |
@t1 you can use the snapshot build. There is a repository you can add https://oss.sonatype.org/content/repositories/snapshots then the snapshot version should be available via maven dependencies |
Hello everyone ...
|
Hi, is there an ETA for the release of this feature? |
This is a follow-up for #495 to migrate @t1 's work to the new filtering APIs.