Aggregate and send coverage report to Codacy

[napoly]

Looking at #4454 it's my turn to fight the code coverage..

This PR uses jacoco plugin with new jacoco report aggregation plugin which requires Gradle 8.x which runs fine with Gradle 7.6 so I needed to upgrade Shadow a bit. The aggregated code coverage xml is generated to /build/reports/jacoco/testCodeCoverageReport/testCodeCoverageReport.xml and has around 34.14MB on my computer. This file is than picked up by the official Codacy coverage reporter action and thus for uploading it needs repository token created by the owner of this repo @bisq-network CODACY_PROJECT_TOKEN (check logs in failed build).

Please note that I could not fully test this PR as I don't have sufficient rights to set token etc.

[napoly]

I created a demo repo to showcase here: https://github.com/napoly/try_git/actions/runs/4958270509 and the reports can be found here: https://app.codacy.com/gh/napoly/try_git/dashboard

[alvasw]

Hi @napoly,
Thank you for doing the research and creating this PR! After reading the Gradle documentation I found out that we don't need to upgrade Gradle or the shadowJar library. I created PRs to support code coverage based on your research. In my PRs, the coverage data is never generated on dev machines by default. We should probably do the same for the shadowJar library because it slows down the build the most.

[napoly]

hi @alvasw it's enough to comment in the PR and I can make the changes.. now it feels you have stolen my work / time here.. at least adding me as a co-authored-by would show some respect imho

[alvasw]

Hi @napoly,

Sorry! I didn't want to steal your work or hurt your feelings!

I checked the Gradle documentation when I reviewed your PR, and I was confused that you upgraded Gradle and the shadow library. We usually only update dependencies if we have to (for security reasons) to avoid supply chain attacks. I had to try it myself to see whether the library updates were needed.

I will close my PRs (#6717, #6718, #6719, #6720) [#6720 needs some changes]. Please cherry-pick the commits and re-commit them (to set yourself as the author). This is the first time, I heard about GitHubs co-author feature.

[napoly]

Hi @alvasw,

no hard feelings here. I do appreciate that you had time and did review this PR thoroughly <3 ! And I don't want to commit your changes as mine.. adding a co-author is a standard and I usually do it while interactively rebasing. So please go ahead and open up those PRs back and just re-word the commit message by adding co-authored-by.

This wasn't and easy task so the story behing upgrading Gradle was that I first though we need at least Gradle > 8.x. Than I realized it is not true so I reverted.. but since I did touch it already I left the minor upgrade there.. (from 7.6 -> 7.6.1). While upgrading Gradle to 8 I had to touch old shadow plugin as well. I left the change there as I though it might be valuable to be up-to-date and since the package name has change there won't be any depend-bot updating it.

As for the PRs you opened... since my PR is open for a longer time you can already go with jacoco 0.8.10. Also you wrote: "In my PRs, the coverage data is never generated on dev machines by default." now watch out here as I'm not completely sure if it's a good thingy as the code coverage can be shown in IDE and can be basically used extensively by a coder.. it looks smthng like:

[alvasw]

Sorry for getting back to you so late!!
I will add you as an co-author and re-open the PRs soon.

Also you wrote: "In my PRs, the coverage data is never generated on dev machines by default." now watch out here as I'm not completely sure if it's a good thingy as the code coverage can be shown in IDE and can be basically used extensively by a coder.. it looks smthng like:

That's a good point. Sadly, I don't think that most developers will find it useful. Most contributors don't write tests anyway.

@HenrikJannsen What do you think?
Enabling coverage data generation for all developers will slow down the build and the build time is already bad.

[HenrikJannsen]

I am against that. The build is already very slow. Just to see the test coverage by default is not justified IMO.

Also tests in Bisq 1 are to a high degree outdated, not well maintained or plainly wrong.
Bisq invested in the past a lot of money on devs who tried to get the test coverage better but it did not end in improvements, rather in the opposite to add more boilerplate (like passing the Clock instance everywhere) or write super complex mock environments which did not correctly represent whats going on.

Also I think resources are better invested in Bisq 2. I personally will not support work in that area anymore, just from the past experience.

[napoly]

I do agree with all you wrote here and also in #6747 (comment). Just to be clear this PR is not trying to force devs to start reaching some coverage threshold or to write unreasonable tests. This feature is provided for free from Codacy and should improve auditability, code understanding, maintainability.. I also agree that this would be even more useful for Bisq 2 as it is in early stage of development.

[HenrikJannsen]

Yes, I was aware that this is just about code coverage reporting in the build. My point was rather that we have to discuss in which areas we want to invest in Bisq 1. I am not arguing to undo that PR, just to not make it default as it slows down the build. But I started the discussion to avoid that new devs spend more efforts in those areas...

[alejandrogarcia83]

I'm closing this pull request in favor of #6750. Feel free to continue the discussion there!