RFC: JMC-5657: Code coverage with Jacoco

Thu Jul 4 08:52:30 UTC 2019

Hi Guru,

Just to let everyone know, here is the updated version:
http://cr.openjdk.java.net/~aptmac/JMC-5657/webrev.03/

The old files are all updated with only "Copyright (c) 2018, 2019, Oracle
and/or its affiliates. All rights reserved."
While the new one have both "Copyright (c) 2019, Oracle and/or its
affiliates. All rights reserved." in the first line and "Copyright (c)
2019, Red Hat Inc. All rights reserved." in the second line.

Thanks,
Carmine

On Tue, Jul 2, 2019 at 6:20 PM Guru <guru.hb at oracle.com> wrote:

> Hi Carmine,
>
> Looks good to me, +1 (tested on Mac OS X) With few Nit
>
> 1.
> http://cr.openjdk.java.net/~aptmac/JMC-5657/webrev.02/raw_files/new/application/coverage/pom.xml
>  and
> http://cr.openjdk.java.net/~aptmac/JMC-5657/webrev.02/raw_files/new/core/coverage/pom.xml
>         In webrev.02 : Copyright (c) 2018, 2019, Oracle and/or its
> affiliates. All rights reserved.
> Expected format : Copyright (c) <File Creation Year>, <File modified
> year>, …
>
> 2. Since you have modified few other files "application/pom.xml,
> application/tests/pom.xml, application/uitests/pom.xml, core/pom.xml and
> core/tests/pom.xml, Please update "Copyright (c) 2018, 2019, Oracle and/or
> its affiliates. All rights reserved.” And you can also add "Copyright (c)
> 2019, Red Hat Inc. All rights reserved.” In the second line
>
> Before pushing the changes, please do update the above comments.
>
> Thanks,
> Guru
>
> On 28-May-2019, at 7:44 PM, Carmine Vincenzo Russo <carusso at redhat.com>
> wrote:
>
> 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
>
>
>

-- 
Carmine Vincenzo Russo