groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul King <pa...@asert.com.au>
Subject Re: Updates on JaCoCo support
Date Mon, 20 Aug 2018 01:28:15 GMT
I'm +1 for adding whatever we need to make coverage work.

Having said that, we shouldn't need to add it for methods (classes?)
already marked synthetic. I also noticed that the kotlin support looks for
a lack of line number information. Could we do something similar? We leave
line number info as -1 in many cases and could do that check just for
classes implementing GroovyObject. Or was this approach ruled out earlier?
The line number approach is more of a hack I guess but we could leave that
as the fall back. That could enable things to almost totally work now and
we can improve incrementally by adding synthetic or @Generated in more
places over time.

Thoughts?

Paul.


On Thu, Aug 16, 2018 at 6:53 AM Andres Almiray <aalmiray@gmail.com> wrote:

> There’s a difference between synthetic and having the @Generated
> annotation, they are not equivalent. Synthetic signals tools to ignore the
> method altogether (JaCoCo honors this behavior). @Generated should be
> applied to non-synthetic methods/classes; what a particular tool decides to
> do with that information is up to the tool itself, in the case of JaCoCo
> it’ll skip the method/class from coverage.
>
> The reason I raises this issue is to see if there are any objections in
> adding @Generated to most (if not all) compiler generated methods (property
> getter/setter, core AST xforms) as this change touches lots of files,
> however it has no impact of Groovy behavior, rather it impacts tools that
> may parse Groovy bytecode (like JaCoCo).
>
> Cheers
> Andres (with no extra a)
>
> Sent from my primitive Tricorder
>
> On 15 Aug 2018, at 22:43, Milles, Eric (TR Technology & Ops) <
> eric.milles@thomsonreuters.com> wrote:
>
> Andreas,
>
>
> One place to start is everywhere that "AnnotatedNode.setSynthetic(true)"
> is currently called.  I think this misses the methods added for a
> property.  But it does cover several AST transforms.  And maybe the
> transforms that add methods and don't call this method could do so in
> addition to the modification to add "@Generated".  Maybe the call to
> setSynthetic could actually add the annotation.  Or you could create a
> utility method that sets synthetic and adds "@Generated".
>
>
> ------------------------------
> *From:* Andres Almiray <aalmiray@gmail.com>
> *Sent:* Tuesday, August 14, 2018 3:42 PM
> *To:* dev@groovy.apache.org
> *Subject:* Updates on JaCoCo support
>
> Hello everyone,
>
> I've spent a couple of hours with JaCoCo team members at the Hackergarten
> Bern this evening.
> The goal of the session was to get started with an integration test for
> the @Generated feature
> added in Groovy 2.5.0.
>
> You can see the outcome at https://github.com/jacoco/jacoco/pull/733
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jacoco_jacoco_pull_733&d=DwMFaQ&c=4ZIZThykDLcoWk-GVjSLmy8-1Cr1I4FWIvbLFebwKgY&r=tPJuIuL_GkTEazjQW7vvl7mNWVGXn3yJD5LGBHYYHww&m=hAfmXOD5R2AP8VF69wcL-UqYZEv2tXAbyBRfDJh0lfg&s=kxj4hAyP-_6cuc12J6CSMgl_xJRqfEjVcnMHfxzID8w&e=>
>
> The good news is that Groovy applies @Generated on constructors added by
> @Cannonical as well
> as methods defined by the GroovyObject interface. The bad news is that the
> test still fails
> because the expectation is that *every* method generated by the compiler
> that does not map
> to a particular source line *should* be annotated with @Generated. The
> following source
>
> ----
> // This annotation generates the following
> // - a constructor that takes an int as argument
> // - a suitable implementation of toString()
> // - a suitable implementation of hashCode()
> // - a suitable implementation of equals(Object)
> // - a public method named canEqual(Object)
> // - a getter & setter for the valRead property
> @groovy.transform.Canonical
> class GroovyDataClassTarget { // assertFullyCovered()
>
>     int valRead // assertNotCovered()
>
>     static void main(String[] args) {
>         new GroovyDataClassTarget() // assertFullyCovered()
>     }
> }
> ----
>
> Generates bytecode equivalent to (decompiled with IntelliJ)
>
> ----
> //
> // Source code recreated from a .class file by IntelliJ IDEA
> // (powered by Fernflower decompiler)
> //
>
> package org.jacoco.core.test.validation.groovy.targets;
>
> import groovy.lang.GroovyObject;
> import groovy.lang.MetaClass;
> import groovy.transform.EqualsAndHashCode;
> import groovy.transform.Generated;
> import groovy.transform.ToString;
> import org.codehaus.groovy.runtime.InvokerHelper;
> import org.codehaus.groovy.runtime.ScriptBytecodeAdapter;
> import org.codehaus.groovy.runtime.callsite.CallSite;
> import org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation;
> import org.codehaus.groovy.runtime.typehandling.ShortTypeHandling;
> import org.codehaus.groovy.util.HashCodeHelper;
>
> @ToString
> @EqualsAndHashCode
> public class GroovyDataClassTarget implements GroovyObject {
>     private int valRead;
>
>     @Generated
>     public GroovyDataClassTarget(int valRead) {
>         CallSite[] var2 = $getCallSiteArray();
>         super();
>         MetaClass var3 = this.$getStaticMetaClass();
>         this.metaClass = var3;
>         this.valRead = DefaultTypeTransformation.intUnbox(valRead);
>     }
>
>     public GroovyDataClassTarget() {
>         CallSite[] var1 = $getCallSiteArray();
>         this(Integer.valueOf(0));
>     }
>
>     public static void main(String... args) {
>         CallSite[] var1 = $getCallSiteArray();
>         var1[0].callConstructor(GroovyDataClassTarget.class);
>     }
>
>     public String toString() {
>         CallSite[] var1 = $getCallSiteArray();
>         Object _result = var1[1].callConstructor(StringBuilder.class);
>         Object $toStringFirst = Boolean.TRUE;
>         var1[2].call(_result,
> "org.jacoco.core.test.validation.groovy.targets.GroovyDataClassTarget(");
>         if (DefaultTypeTransformation.booleanUnbox($toStringFirst)) {
>             Boolean var4 = Boolean.FALSE;
>         } else {
>             var1[3].call(_result, ", ");
>         }
>
>         var1[4].call(_result, var1[5].callStatic(InvokerHelper.class,
> var1[6].callCurrent(this)));
>         var1[7].call(_result, ")");
>         return
> (String)ShortTypeHandling.castToString(var1[8].call(_result));
>     }
>
>     public int hashCode() {
>         CallSite[] var1 = $getCallSiteArray();
>         Object _result = var1[9].callStatic(HashCodeHelper.class);
>         if
> (!DefaultTypeTransformation.booleanUnbox(var1[10].call(var1[11].callCurrent(this),
> this))) {
>             Object var3 = var1[12].callStatic(HashCodeHelper.class,
> _result, var1[13].callCurrent(this));
>             _result = var3;
>         }
>
>         return DefaultTypeTransformation.intUnbox(_result);
>     }
>
>     public boolean canEqual(Object other) {
>         CallSite[] var2 = $getCallSiteArray();
>         return other instanceof GroovyDataClassTarget;
>     }
>
>     public boolean equals(Object other) {
>         CallSite[] var2 = $getCallSiteArray();
>         if (ScriptBytecodeAdapter.compareEqual(other, (Object)null)) {
>             return false;
>         } else if
> (DefaultTypeTransformation.booleanUnbox(var2[14].callCurrent(this, other)))
> {
>             return true;
>         } else if (!(other instanceof GroovyDataClassTarget)) {
>             return false;
>         } else {
>             GroovyDataClassTarget otherTyped =
> (GroovyDataClassTarget)other;
>             if
> (!DefaultTypeTransformation.booleanUnbox(var2[15].call(otherTyped, this))) {
>                 return false;
>             } else {
>                 return
> ScriptBytecodeAdapter.compareEqual(var2[16].callCurrent(this),
> var2[17].call(otherTyped));
>             }
>         }
>     }
>
>     public int getValRead() {
>         return this.valRead;
>     }
>
>     public void setValRead(int var1) {
>         this.valRead = var1;
>     }
> }
> ----
>
> We can appreciate that the methods added by @ToString, @EqualsAndHashcode,
> and the property getter/setter are not
> annotated with @Generated, which will prompt JaCoCo to mark them as not
> covered. The rationale from the JaCoCo team
> is that these methods should be annotated as the compiler is "trusted",
> only those methods explicitly added to the
> source should be covered.
>
> Thus, here comes the call to action and the reason why I wanted to start
> this conversation in the first place:
>  - modify the Groovy compiler to add @Generated on property getters and
> setters.
>  - modify core AST xforms to add @Generated where it makes sense.
>
> Related to the original @Generated issue (as commented by Evgeny at
> https://github.com/jacoco/jacoco/pull/610
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jacoco_jacoco_pull_610&d=DwMFaQ&c=4ZIZThykDLcoWk-GVjSLmy8-1Cr1I4FWIvbLFebwKgY&r=tPJuIuL_GkTEazjQW7vvl7mNWVGXn3yJD5LGBHYYHww&m=hAfmXOD5R2AP8VF69wcL-UqYZEv2tXAbyBRfDJh0lfg&s=44Nu_515VJUtJ8wIVcQJF2ow_axTotUprpo9KwRmQMY&e=>)
> fields
> do not have line numbers, would be good to have them.
>
> What do you think?
>
> Cheers,
> Andres
>
> -------------------------------------------
> Java Champion; Groovy Enthusiast
> JCP EC Associate Seat
> http://andresalmiray.com
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__andresalmiray.com&d=DwMFaQ&c=4ZIZThykDLcoWk-GVjSLmy8-1Cr1I4FWIvbLFebwKgY&r=tPJuIuL_GkTEazjQW7vvl7mNWVGXn3yJD5LGBHYYHww&m=hAfmXOD5R2AP8VF69wcL-UqYZEv2tXAbyBRfDJh0lfg&s=ynGizu7RDzXxq8Mp65SEUYPVyKHTo7kc14uNurHMLRM&e=>
> http://www.linkedin.com/in/aalmiray
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.linkedin.com_in_aalmiray&d=DwMFaQ&c=4ZIZThykDLcoWk-GVjSLmy8-1Cr1I4FWIvbLFebwKgY&r=tPJuIuL_GkTEazjQW7vvl7mNWVGXn3yJD5LGBHYYHww&m=hAfmXOD5R2AP8VF69wcL-UqYZEv2tXAbyBRfDJh0lfg&s=jMgNE8ncoIZmIOY8tu6FwSSs_8KA4Vh6-7GpRqDNEd4&e=>
> --
> What goes up, must come down. Ask any system administrator.
> There are 10 types of people in the world: Those who understand binary,
> and those who don't.
> To understand recursion, we must first understand recursion.
>
>

Mime
View raw message