RFC: JMC-5657: Code coverage with Jacoco

Tue May 28 14:14:50 UTC 2019

Hi everyone,

I took sometimes to fix all the issues, here is the update patch:
http://cr.openjdk.java.net/~aptmac/JMC-5657/webrev.02/

I  have made some comments inline, but for a easier reading here's a
summarized version of everything in the patch:
- Two new pom.xml: ~/jmc/core/coverage/pom.xml and
~/jmc/application/coverage/pom.xml
- ~/jmc/core/coverage/pom.xml gets executed during the *mvn clean install*
step to build JMC itself
- ~/jmc/application/coverage/pom.xml gets executed both with *mvn verify*
and *mvn verify -P uitests*
- If you run just *mvn verify* the coverage will only show the percentage
for ~/jmc/application/tests, while if you run *mvn verify -P uitests *the
coverage will show both the percentage in ~/jmc/application/tests and
~/jmc/application/uitests in the same report.
- All the modules in ~/jmc/applicatin/tests and ~/jmc/application/uitests
are dependencies in ~/jmc/application/coverage/pom.xml
- To make sure that everything works fine, the profile *uitests* is also in
~/jmc/application/coverage/pom.xml, this way the modules from
~/jmc/application/uitests get loaded only when the profile uitests is
executed
- Both .gitignore and .hgignore got updated to avoid
~/jmc/*/coverage/coverage-report
- In ~/jmc/application/pom.xml all the modules are uncommented and working
now
- Every unrelated and useless comment has been removed

>>> 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.
>>>
>>
With Alex's help, I found out there was a problem with having directly a
unique coverage report for both ~/jmc/application/tests and
~/jmc/application/uitests.
Because of this now we still have a unique report generated with
~/jmc/application/coverage/pom.xml but inside there is a profile *uitests*.
This way all the dependencies needed for the uitests are only considered
when the uitests are executed. Meanwhile with a simple *mvn verify* you get
the coverage only for ~/jmc/application/tests.

> > > 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
>>
>
All useless and unrelated comment have been removed or fixed to make
everything works.

>
>> 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/*.
>>
>
Both .gitignore and .hgignore got updated to ignore
~/jmc/*/coverage/coverage-report/

>
> Whoops, spoke a tad too soon.
>
> It's having difficulties at the moment when running uitests.
> https://travis-ci.org/aptmac/jmc-qa/builds/535820411
>

Apparently this was because of some reference to the old name *uireport*
still around that I didn't notice, it's been taken care of now.

> There's also comments in the application/coverage/pom.xml for the update
> sites and RCP product.
>

All the modules now are uncommented and working, for these three an
additional tag was needed because those repositories only have pom files
and not jar files.

Thanks,
Carmine

-- 
Carmine Vincenzo Russo