Add Java code coverage reporting #686
Conversation
ches
requested review from
davidheryanto,
khorshuheng,
pradithya,
woop and
zhilingc
as code owners
| <dependency> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-core</artifactId> | ||
| <version>2.23.0</version> |
There was a problem hiding this comment.
A recent Byte Buddy is needed for some functionality of Mockito with Java 11, which is probably why we already have a 1.9.x version set in root POM dependency management. For some reason this older version of Mockito overriding in a couple of modules breaks even though the new Byte Buddy version should be forced, specifically when JaCoCo instrumentation is running. Letting these use newer Mockito set in dependency management already fixed it. This seems to be a thing.
It appears there is no issue with the newer Mockito on these snowflake modules anymore anyway, so good to drop the special cases.
|
Thanks for this @ches. Coverage is pretty poor. That BQ connector :(
Anything you add to Relevant code https://github.com/gojek/feast/blob/master/Makefile#L153 Example: https://api.docs.feast.dev/python/ I am happy to merge as is. Would be nice to have a report somewhere, but not sure if I want to advertise this badge yet. |
|
/retest |
|
I added simple persistence of the report as a build artifact. They are not conveniently browseable, but it's something at least, I'm happier to have that than not. I'm more interested in knowing what is covered and what isn't, more than an aggregate percentage number, so I don't much care about a badge anyway. We should also bear in mind this is unit tests only (and some tests with in-memory DBs that I'd consider a level of integration). The lint-python check is failing on master and not affected by this PR. |
|
/retest Even though e2e shouldn't be failing from this PR, I guess having it pass can eventually let Prow do the merge… |
|
Rebased with master for the Python linting fix. |
ches
added
the
backport-candidate
|
/assign @woop |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ches, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
/retest |
|
We are going back to peak flakiness. Apologies, people are looking at this. At least 3 different flaky tests. |
What this PR does / why we need it:
I'm interested in tracking coverage data. I thought you might be too.
This adds the JaCoCo Maven plugin, with a module dedicated to reporting aggregate coverage, which is said to be the most reliable way to achieve that. It is currently under
docs/coverage/javaas a tidy place to tuck it away, but GitBook might barf on apom.xmlbeing in there, not sure, so I'll move it if you suggest someplace else.We use Codecov privately, it should slurp up JaCoCo data readily if you're interested in setting it up, and/or I can look into adding a GitHub Action with JaCoCo's stock HTML report kept as an artifact.
I don't have convenient access to a public web host at the moment to drop the static browseable report as an example, so here's a screenshot of the root page for where we stand:
You can generate it yourself on the branch with:
Does this PR introduce a user-facing change?: