Emit a groovy @Generated annotation on all generated methods and fields so jacoco can ignore them in relation to coverage

[demus-nine]

Steps to Reproduce

1. Run jacoco to generate a coverage report
2. Examine the results calculated fro grails artifacts
3. They are very low, because a lot of boilerplate methods are not normally exercised by unit tests

Expected Behaviour

Coverage should only consider manually created code, when calculating coverage.

Actual Behaviour

Coverage results for grails artifacts is in the tens, skewing the results so much they are unusable.

Groovy already adds @Generated annotations, so non-grails classes have much, much higher coverage even though there is a lot of metadata-related code attached to each class.

Environment Information

• Operating System: Any that runs jacoco
• Grails Version: 4
• JDK Version: Any

Example Application

Any application will do. Just generate a jacoco report.

[jessie-apollotech]

java version: 8.0.222-amzn grails version: 4.0.0

here is how I reproduced this issue:

• creates a grails 4 app
• appends jacoco config to build.gradle
• create a domain class
• create a test for domain class
• run jacoco test report task
• view output in firefox

sequence of commands to produce jacoco report:

grails create-app foo-app && cd foo-app
cat >> build.gradle << 'EOF'
apply plugin:"jacoco"
jacoco {
    toolVersion = "0.8.4"
}
jacocoTestReport {
    dependsOn test
    reports {
        xml.enabled false
        csv.enabled false
        html.destination file("${buildDir}/reports/jacocoHtml")
    }
}
EOF
mkdir -p grails-app/domain/foo/app/
cat >> grails-app/domain/foo/app/Bar.groovy << EOF
package foo.app

class Bar {
    String name

    static constraints = {
    }
}
EOF
mkdir -p src/test/groovy/foo/app/
cat >> src/test/groovy/foo/app/BarSpec.groovy << EOF
package foo.app

import grails.testing.gorm.DomainUnitTest
import spock.lang.Specification

class BarSpec extends Specification implements DomainUnitTest<Bar> {

    def setup() {
    }

    def cleanup() {
    }

    void "test should have name property"() {
        Bar bar = new Bar()
        bar.name = "valid"
        expect:"name should be retained"
        "valid".equals(bar.name)
    }
}
EOF
firefox build/reports/jacocoHtml/index.html

[jessie-apollotech]

I'd like to try my hand on this. Can someone point me to the code that injects the methods to a domain class?

[aulea]

Maybe should wait with this groovy3 - https://issues.apache.org/jira/browse/GROOVY-8765

[demus-nine]

The groovy version doesn't really make a difference, I would think. It's the grails generated methods, fx. on entities. that need have the annotations added, and those are generated by grails, as I understand it. Also not everyone is immediatly doing a major upgrade of groovy and it would be nice to be able to do coverage.

[demus-nine]

GROOVY-8765 was merged into groovy 3.x in https://github.com/apache/groovy/commit/44ef38bcab9e906ff8757cb8ba68d679d5ffd527 and into groovy 2.5.x in https://github.com/apache/groovy/commit/c34ba74518e71c5badb4108807c1da60f3581f91 . As I was using groovy 2.5.6 at the time, the referenced issue did not solve the original problem. I am guessing a markAsGenerated() call needs to be added to the various ast transformers in grails for the annotation to be added.

[jessie-apollotech]

@demus-nine have you had any luck with getting good coverage reports? we've been using grails since pre-1.0 and would like to stay with it but its hard to recommend over just plain spring boot without proper code coverage.

[demus-nine]

The grails generated methods still give low coverage, because there are a whole lot of them. You can do some manual adjustments to get a usable number, and you can use the changes in coverage numbers to flag an unfortunate direction of travel, but you don't notice gradual changes, because it's lost in the noise of generated methods.

[aulea]

@puneetbehl its not finished yet, similar changes must be done at least in GORM and GORM-Hibernate. I will do, but it takes time and this was sample to verify if its okay solution.

[puneetbehl]

I would suggest to open issues in both GORM, GORM Hibernate5, and also link the same here.

[tlefevre]

It's because of things like this that Grails is slowly dying. Code coverage is an essential tool, lacking one makes it very hard to recommend this in Enterprise setup besides just personally being a big turnoff.

Openclover is dead as well, that used to be pretty good.

[andersaaberg]

Now that OpenClover seems dead and is incompatible with Groovy 3 + Grails 5, then we desperately need to fix Jacoco coverage for Grails in order to upgrade to Grails 5. One of my Grails 4 projects had 95% coverage with OpenClover and dropped to 45% instruction coverage + 34% branch coverage with Jacoco after trying to upgrade to Grails 5.

[andersaaberg]

Many thanks for the fixes @aulea - with Grails 5.1.2, coverage has improved to 66% instruction coverage and 69% branch coverage! However, it seems that many getters and setters are not excluded from Jacoco even though they have @Generated:

Maybe it would work to add @Generated to the private fields that the getters and setters are for? Once we have that fixed, then we can finally switch to Grails 5.

[aulea]

Those might come from groovy, how it handles Traits and their private fields. For some cases I managed to overcome, when I created by myself getters and setters. Will dig in and try to find out, whether have to handle them on grails or groovy side. ... Created issue on groovy side https://issues.apache.org/jira/browse/GROOVY-10505 and will provide pull-request to there

[andersaaberg]

@aulea : Many thanks, I think that you are right that the fix needs to be made in Groovy. I tried to add @Generated on the private fields in RequestForwarder, ResponseRedirector, ResponseRenderer in grails-core, but the jacoco coverage did not improve when using the local release of grails-core. It didn't help to create @Generated getters for those private fields either.

The bytecode still has the following methods that does not have @Generated:

public boolean grails_artefact_controller_support_ResponseRedirector__useJsessionId$get()
public boolean grails_artefact_controller_support_ResponseRedirector__useJsessionId$set(boolean val)

[aulea]

There should be now new releases of groovy with this fix (2.5.16, 3.0.10, 4.0.1)

[andersaaberg]

@aulea : gr8, the instruction coverage increased: from 66% to 81%. However, there is still some work needed.

I have a Controller, which takes a command object:

class TestController {
  def doSomething(SomeCommand someCommand) {
   ...
  }
}  

Then the compiled TestController.class contains this following bytecode:

    @DelegatingMethod
    public Object doSomething(SomeCommand param1) {
        // $FF: Couldn't be decompiled
    }

    @Action(
        commandObjects = {SomeCommand.class}
    )
    public Object doSomething() {
        // $FF: Couldn't be decompiled
    }

The first method is expected to not have @groovy.transform.Generated, but the second one should have @groovy.transform.Generated. This is only a problem for controller methods that takes a parameter and hence uses @DelegatingMethod.

Furthermore, the controller bytecode also contains this method that should have @groovy.transform.Generated:

    public void render(Converter<?> arg1) {
...
    }

[aulea]

did pull-request for ControllerActionTransformer.

The render method is from grails-plugin-converters RenderConverterTrait. Maybe will do pull-request to there also.

[puneetbehl]

There has been discussion around it in the Grails Engineering Meeting and the argument is that the generated code is actually considered a part of the application. So, it was discussed that by default the @Generated annotation should NOT be added and a configurable option should be allowed to change this behavior.

[mlkristiansen]

@puneetbehl I am reading this as you think the framework testing should be done by everyone using the framework, and not just by framework developers? (I hope I am misunderstanding you)

[andersaaberg]

@puneetbehl : then we need to set that configuration for all of our projects - that does not really seem to adhere to "convention over configuration". How come can the default not be to add @Generated? Then it can be disabled for the corner cases (e.g. testing the framework).

[sbglasius]

I agree with @andersaaberg, as developer using Grails we should not opt-in to run code coverage for parts not related to our own application. For coverage for Grails itself it should opt-out instead

[boardbloke]

+1 for this we have upgraded to Grails 5 but we have to justify the code coverage metrics to product managers on every release. Its not pleasant.

Also - I noticed that some PRs made by @aulea that @puneetbehl added to the 7.0.x branch of data-mapping in https://github.com/grails/grails-data-mapping/issues/1445 haven't been applied to later versions, so its not available in Grails 5.1+. I raised an issue about that a while ago - https://github.com/grails/grails-data-mapping/issues/1662.

[fernando88to]

I'm upgrading an application from grails 2 to grails 5 and I chose to use open clover even though it doesn't support groovy 3. I have some problems but the coverage is better than jacoco.

[demus-nine]

There has been discussion around it in the Grails Engineering Meeting and the argument is that the generated code is actually considered a part of the application. So, it was discussed that by default the @Generated annotation should NOT be added and a configurable option should be allowed to change this behavior.

While the generated code is indeed part of the application, it is also undeniably generated. The generated annotation doesn't make any assertions about the frameworkishness or applicationness of the generated code, just that is generated. The whole point is to not include generated code in coverage, as the correctness of the code-generator is presumed to be verified in and of itself.

[puneetbehl]

After thinking about my previous comment, I think adding a "generated" annotation would be a good idea, which simply means it's a generated code.

Currently, I am working on the corresponding pull request, please check my comments for more information.

[davebrown1975]

Do we need to do something after upgrading to 5.3.0 to see the fixes for this issue? After cleaning, generating the tests and rerunning the report, I'm still seeing exactly the same issues, thousands of entries for uncovered code same as apollotech reported at the start of the comments for this issue back in 2019.

[aulea]

Given snapshot seems to be from Domain class.

GORM 7.3.x branch is missing the commit https://github.com/grails/grails-data-mapping/commit/ed6d4740f2d80d64b78d4aa1ec645c20afe1e3ba It's only in 7.0.x branch

[boardbloke]

I raised that as https://github.com/grails/grails-data-mapping/issues/1662 a while ago