RFC: JMC-5657: Code coverage with Jacoco
Guru
guru.hb at oracle.com
Thu Jul 4 10:05:48 UTC 2019
+1 @webrev.03
Thanks,
Guru
> On 04-Jul-2019, at 2:22 PM, Carmine Vincenzo Russo <carusso at redhat.com> wrote:
>
> Hi Guru,
>
> Just to let everyone know, here is the updated version: http://cr.openjdk.java.net/~aptmac/JMC-5657/webrev.03/ <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 <mailto: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 <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 <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 <mailto: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/ <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 <mailto: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 <mailto: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 <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/ <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 <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
More information about the jmc-dev
mailing list