RFC: JMC-5657: Code coverage with Jacoco

[Alex Macdonald]

Hi Carmine,

On Fri, May 10, 2019 at 2:27 PM Carmine Vincenzo Russo <carusso at redhat.com>
wrote:

> Hi Guru,
>
> thanks for your reply.
>
> On Tue, May 7, 2019 at 6:38 PM Guru <guru.hb at oracle.com> wrote:
>
> > Hi Carmine,
> >
> > Please check my comments inline.
> >
> > Thanks,
> > Guru
> > > On 07-May-2019, at 2:05 PM, Carmine Vincenzo Russo <carusso at redhat.com
> >
> > wrote:
> > >
> > > Hello everyone,
> > >
> > > This patch adds Jacoco coverage over unit test in JMC.
> > > The module covered in particular are: /core/tests and
> /application/tests.
> > >
> > > The patch addresses the issue JMC-5657[0], but as noted by Klara
> Ward[1],
> > > instead of adding Jcov, it's better to add Jacoco.
> > >
> > > For the time being, each test group had a new module, report, where an
> > > aggregate-report can be found.
> > > You need to run 'mvn verify' in jmc root for /application/tests and in
> > > /core for /core/tests to have the coverage report.
> > Logically there are three build step
> > 1. For releng
> > 2. Core modules
> > 3. Jmc product (i.e Application)
> > Ideally `mvn verify` is run on ~/jmc/core (for step 2) and ~/jmc (for
> step
> > 3).
>
>
> Obviously all the 3 steps are needed, I also checked, the 'mvn verify' for
> ~/jmc/core is automatically done while building the core modules on step 2.
> For jmc product you still need to run 'mvn verify -P uitests' separately
> after building the project since you need the test to be executed.
>
> Unless there is restriction on defining Jacoco in root of
> > ~/jmc/core/pom.xml and ~/jmc/pom.xml.
> >
>
> I explain this at the end.
>
>
> > > There are also some aspects that concern me, so: Is this what we are
> > > looking for?
> > > The report for /core[2] looks good and clean. The report for
> > > /application/tests[3][4] has good results, although there are some 0%,
> > and
> > > that is ok, but also some N/A. Should we keep the modules in the report
> > > with only N/A?
> > >
> > > There are also some changes in /application/uitests when reading the
> > patch,
> > > they are not yet applied but I would like to add them to the next
> patch.
> > Please file a follow on new JBS and link with old one (relates to
> > JMC-5667).
> >
>
> With 'next patch' I just meant the updated patch after the RFC, basically
> what I have done now, just wanted to have your opinions before going
> forward with the ~/uitests coverage. If a follow up issue is needed, I'll
> ask Alex to open it.
>
>
> > > This brings me to my second concern: should the tests and uitests in
> > > /application have a unique report or should I keep them separate?
> > Good to have Unique rather than separate.
> >
>
> In the new patch attached ~/jmc/application/tests and
> ~/jmc/applciation/uitests have a unique coverage in
> ~/jmc/application/coverage.
>
>
> > > Lastly, do we want coverage with every verify build or do we want a
> maven
> > > profile/flag to have the coverage only in some occasions?
> > This could be based on how much time Jacoco Code coverage takes. If its
> > negligible then good to have on every verify build or with a
> profile/flag.
> >
>
> In all my tests and attempts the module itself never took more than 10s,
> while the build doesn't seem to be affected. So basically I don't think
> it's a problem to have always the coverage in every build.
>

I took a quick look into this and ran `mvn verify` locally with and without
the coverage patch, I saw:

With Coverage:
core - 1 minute 17 seconds
jmc - 8 minutes 51 seconds

Without Coverage:
core - 1 minute 3 seconds
jmc - 8 minutes 23 seconds


>
> > > Overall, how does the patch look like? Do you think there are some more
> > > modules that need to be covered? And do you have any suggestions on how
> > to
> > > improve what I've done?
> > If possible, please update the patch to ~/jmc/core/pom.xml and
> > ~/jmc/pom.xml.
> >
>
> As far as my knowledge goes, to have the report, we need a separate pom.xml
> for the report, this because the report-aggregate[0] relies on <dependency>
> and not on <module>, even the JaCoCo examples for this have their own
> report module[1]. The best way, I think, is the current situation in which
> we have ~/jmc/core/coverage/pom.xml and ~/jmc/application/coverage/pom.xml,
> these generate and contains the coverage report, while in
> ~/jmc/core/pom.xml and ~/jmc/application/pom.xml we have the JaCoCo agent
> that makes the coverage possible. If we try to move everything in
> ~/jmc/application/pom.xml and ~/jmc/core/pom.xml the reports will be
> segregated in their own modules. Or if we try to move part of it in
> ~/jmc/pom.xml we stiil need to have both the agent configuration in
> ~/jmc/application/pom.xml and ~/jmc/core/pom.xml and the coverage as
> separate modules.
>

The aggregation of the coverage reports looks like it will require it's own
pom.xml, as exemplified in a sample demo from the JaCoCo github.
https://github.com/jacoco/jacoco/tree/v0.7.8/jacoco-maven-plugin.test/it/it-report-aggregate


>
> All the changes and the new addition can be found here:
> http://cr.openjdk.java.net/~aptmac/JMC-5657/webrev.01/
>

core/tests/pom.xml
- <module>report</module> is commented out

Overall I think the patch looks good. I would however like to see the
.hgignore and .gitignore updated to exclude the coverage reports; there are
a lot of files generated in */coverage/coverage-report/*.

Regards,
> Carmine
>
> [0] https://www.jacoco.org/jacoco/trunk/doc/report-aggregate-mojo.html
> [1]
>
> https://github.com/jacoco/jacoco/tree/v0.7.8/jacoco-maven-plugin.test/it/it-report-aggregate
>
> > Thanks,
> > > Carmine
> > >
> > > [0] https://bugs.openjdk.java.net/browse/JMC-5657
> > > [1]
> > http://mail.openjdk.java.net/pipermail/jmc-dev/2019-March/000882.html
> > > [2] https://imgur.com/hPyyLsQ
> > > [3] https://imgur.com/fqdzGvH
> > > [4] https://imgur.com/ozqNcAB
> > >
> > > --
> > > Carmine Vincenzo Russo
> > > <5657-0.patch>
> >
> >
>
> --
> Carmine Vincenzo Russo
>