[improve][CI] Generate unit test code coverage reports and upload to Codecov #17382

yaalsn · 2022-09-01T01:41:55Z

Motivation

Add code coverage to make sure good quality of PRs. Although unit tests run in different runners, by using Codecov the coverage XML reports can be aggregated automately.

We can add the coverage to take a look first and then adjust the rule of coverage.

Verifying this change

Make sure that the change passes the CI checks.

Documentation

Check the box below or label this PR directly.

Need to update docs?

doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

Matching PR in forked repository

PR in forked repository: https://github.com/yaalsn/pulsar/pull/7

yaalsn · 2022-09-01T02:09:52Z

https://github.com/codecov/sourcegraph-codecov/blob/master/README.md This Chrome ext can show code coverage information from Codecov on GitHub directly which means no need to jump to Codecov to take look.

tisonkun · 2022-09-01T02:16:38Z

@yaalsn can you share the report of Pulsar right now? I'm afraid that we just add one more workflow while no one cares about it especially when the result is overwhelming all consumers.

yaalsn · 2022-09-01T02:22:41Z

@tisonkun Thanks for your reminder.

https://app.codecov.io/gh/apache/pulsar/tree/add-code-coverage This is this PR's coverage report, and Codecov will comment a message to this PR's body after the CI uploads the report.

nodece

LGTM

tisonkun

It's a significant change to run Pulsar CI every commit on master. Comments inline.

Except that, the new steps take a few seconds to complete. I'm +0 to introduce it. Although, I'd say I never read a coverage report and don't find it helpful. If anyone who gets benefit from such a report instead of be bothered, please let me know (how).

tisonkun · 2022-09-01T04:09:20Z

.github/workflows/pulsar-ci.yaml

@@ -25,6 +25,7 @@ on:
  push:
    branches:
      - branch-*
+      - master


I don't think we should run tests for every commit on the master. If you want to generate a codecov report every day, you may schedule a daily cron job. Anyway, this is a significant change to the CI workflow.

cc @codelipenghui @lhotari

IMO, for each PR, we should only generate the codecov report for the new changes. I'm not sure does codecov can support?

I don't find any configuration on report for the new changes except require_changes in comment selection and patch in coverage/status section, but It seems they are not what @codelipenghui wants. Could you help to take a look again?

https://docs.codecov.com/docs/codecovyml-reference

nicoloboschi

I started the same work a couple of months ago here #14600 - my pull is no more valid because I did with the old CI
but I suggest you to add the jacoco config file (see my pull) and I will close mine

Also we need to discuss it on dev@
we need to document why it's important, how to see it and what are the future plans.
That was my thread discussion https://lists.apache.org/thread/o002mmb85jf2948ffq0fsgmg3cvbmh64

you can start replying to that

eolivelli

Thank you for your initiative @yaalsn !

I agree with @tisonkun and with @nicoloboschi
we should discuss on dev@ and if the community really wants to go this path we must define a work flow.

I have seen (and also promoted) a few times to add Codecov or other similar tools in other OSS projects.
it is pretty hard to make it useful, especially with big projects like Pulsar.

yaalsn · 2022-09-15T00:05:42Z

I added some configuration to codecov.yml in my repo's pulsar because of the apache/pulsar's runner resources lacking. This is the result: https://github.com/yaalsn/pulsar/pull/11

We can get the uncovered part from Files Changed

The coverage and impacted files general information in comment

Status check in action check, but it doesn't block the merge even if the coverage down

yaalsn · 2022-09-15T02:10:06Z

@tisonkun @nicoloboschi Could you take a look at the coverage data again? It looks good to me, what do you think?

tisonkun

Rest LGTM. Since it doesn't affect the CI workload a lot, it's OK we fire the gun and give this report a try.

tisonkun · 2022-09-15T02:14:03Z

pom.xml

-                  <outputDirectory>target/report</outputDirectory>
+                  <excludes>
+                    <exclude>**/*.jar</exclude>
+                  </excludes>


Why do you make this change?

We just need to scan class file code coverage not jar files'

And it may lead to some error if we scan the jar files.

codecov.yml

tisonkun · 2022-09-15T05:04:17Z

@yaalsn interesting. The result seems not quite correct or useful.

tisonkun · 2022-09-15T05:19:01Z

And it seems quite noisy. Perhaps some settings are incorrect.

codecov-commenter · 2022-09-15T05:19:36Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2b9ffac). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #17382   +/-   ##
=========================================
  Coverage          ?   45.92%           
  Complexity        ?    17525           
=========================================
  Files             ?     1569           
  Lines             ?   127638           
  Branches          ?    14038           
=========================================
  Hits              ?    58614           
  Misses            ?    63028           
  Partials          ?     5996

Flag	Coverage Δ
unittests	`45.92% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

yaalsn · 2022-09-15T08:13:31Z

@yaalsn interesting. The result seems not quite correct or useful.

@tisonkun Because there are some flaky tests, and if the job run success, the result will be different. If we want to turn off the email notification, we need to disable the comment from codecov too.

Another way is to change the behaviour to default, and it will send a email at the first time. (but it is not the final result)

yaalsn · 2022-09-15T14:13:16Z

/pulsarbot rerun-failure-checks

yaalsn · 2022-09-16T07:15:48Z

/pulsarbot rerun-failure-checks

yaalsn · 2022-09-18T12:41:03Z

/pulsarbot rerun-failure-checks

yaalsn · 2022-09-19T02:01:09Z

/pulsarbot rerun-failure-checks

nodece · 2022-09-19T03:24:09Z

Hi @yaalsn, the CI looks broken, we should rerun all jobs.

Error: Failed to execute goal on project pulsar-all-docker-image: Could not resolve dependencies for project org.apache.pulsar:pulsar-all-docker-image:pom:2.11.0-SNAPSHOT: The following artifacts could not be resolved: org.apache.pulsar:pulsar-io-distribution:pom:2.11.0-SNAPSHOT, org.apache.pulsar:pulsar-offloader-distribution:tar.gz:bin:2.11.0-SNAPSHOT: org.apache.pulsar:pulsar-io-distribution:pom:2.11.0-SNAPSHOT was not found in https://repository.apache.org/snapshots during a previous attempt. This failure was cached in the local repository and resolution is not reattempted until the update interval of apache.snapshots has elapsed or updates are forced -> [Help 1]

…eCov

Co-authored-by: tison <[email protected]>

…he email notification times.

…est_group_other

yaalsn · 2022-09-21T09:36:46Z

/pulsarbot rerun-failure-checks

yaalsn · 2022-09-21T11:14:54Z

/pulsarbot rerun-failure-checks

codelipenghui · 2022-09-27T03:06:08Z

build/run_unit_group.sh

@@ -129,7 +129,7 @@ function test_group_proxy() {

 function test_group_other() {
  mvn_test --clean --install \
-           -pl '!org.apache.pulsar:distribution,!org.apache.pulsar:pulsar-offloader-distribution,!org.apache.pulsar:pulsar-server-distribution,!org.apache.pulsar:pulsar-io-distribution' \
+           -pl '!org.apache.pulsar:distribution,!org.apache.pulsar:pulsar-offloader-distribution,!org.apache.pulsar:pulsar-server-distribution,!org.apache.pulsar:pulsar-io-distribution,!org.apache.pulsar:pulsar-all-docker-image' \


Does this change related to this PR?

If we don't exclude org.apache.pulsar:pulsar-all-docker-image, then there will appear this error.

Although I think it's something related to network, it does no harm to exclude org.apache.pulsar:pulsar-all-docker-image in test_group_other.

codelipenghui · 2022-10-10T00:52:40Z

@tisonkun @nicoloboschi @eolivelli Could you please help review this PR again?

tisonkun

Good to go :)

tisonkun · 2022-10-10T01:14:23Z

build/run_unit_group.sh

@@ -129,7 +129,7 @@ function test_group_proxy() {

 function test_group_other() {
  mvn_test --clean --install \
-           -pl '!org.apache.pulsar:distribution,!org.apache.pulsar:pulsar-offloader-distribution,!org.apache.pulsar:pulsar-server-distribution,!org.apache.pulsar:pulsar-io-distribution' \
+           -pl '!org.apache.pulsar:distribution,!org.apache.pulsar:pulsar-offloader-distribution,!org.apache.pulsar:pulsar-server-distribution,!org.apache.pulsar:pulsar-io-distribution,!org.apache.pulsar:pulsar-all-docker-image' \


Although I think it's something related to network, it does no harm to exclude org.apache.pulsar:pulsar-all-docker-image in test_group_other.

tisonkun · 2022-10-10T01:15:52Z

pom.xml

-                  <outputDirectory>target/report</outputDirectory>
+                  <excludes>
+                    <exclude>META-INF/**</exclude>
+                  </excludes>


I don't know the reason you made this change. Could you explain it a bit @yaalsn?

The last time you changed to **/*.jar and now a new change, we need some background here.

However, given that the CI result looks good, I assume this change is OK.

@tisonkun You can take a look at this one. Third-party classes and jars are in target/META-INF path and may lead to some errors, so we need to exlude this path.

@yaalsn Thank you. One more question: why remove <outputDirectory>target/report</outputDirectory>?

@tisonkun Because I think the default value makes sense: target/jacoco directory means jacoco reports but target/report means multi kinds of reports.

@tisonkun Thanks for your comments! I think all the questions are very good which I met during this PR, others see the discussion can know clearly about these.

eolivelli

+1

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 1, 2022

nodece approved these changes Sep 1, 2022

View reviewed changes

nodece added the area/ci label Sep 1, 2022

tisonkun requested changes Sep 1, 2022

View reviewed changes

nicoloboschi requested changes Sep 1, 2022

View reviewed changes

eolivelli requested changes Sep 1, 2022

View reviewed changes

tisonkun reviewed Sep 15, 2022

View reviewed changes

yaalsn closed this Sep 19, 2022

yaalsn reopened this Sep 19, 2022

yaalsn closed this Sep 20, 2022

yaalsn reopened this Sep 21, 2022

yaalsn added 5 commits September 21, 2022 13:56

[improve][ci] Generate jacoco code coverage reports and upload to Cod…

7248378

…eCov

[improve][ci] Generate jacoco code coverage reports and upload to Cod…

e0a3ab1

…eCov

[improve][ci] Generate jacoco code coverage reports and upload to Cod…

4b2e94f

…eCov

[improve][ci] Generate jacoco code coverage reports and upload to Cod…

6a221d2

…eCov

Add codecov configuration file

ac414d5

yaalsn and others added 7 commits September 21, 2022 13:56

Update codecov.yml

dd0736f

Co-authored-by: tison <[email protected]>

Change codecov comment behaviour configuration to default to reduce t…

39e9c9f

…he email notification times.

Allow CodeCov upload report job can be failure

7edc669

Include only apache/pulsar class to generate jacoco tests

65d2546

Include only apache/pulsar class to generate jacoco tests

f1f115d

Exclude class files in META-INF and skip pulsar-all-docker-image in t…

ce950f0

…est_group_other

Remove generate jacoco reports step in pulsar-ci

83d153a

codelipenghui added the ready-to-test label Sep 21, 2022

codelipenghui assigned yaalsn Sep 21, 2022

codelipenghui closed this Sep 21, 2022

codelipenghui reopened this Sep 21, 2022

codelipenghui reviewed Sep 27, 2022

View reviewed changes

tisonkun approved these changes Oct 10, 2022

View reviewed changes

nicoloboschi approved these changes Oct 10, 2022

View reviewed changes

eolivelli approved these changes Oct 11, 2022

View reviewed changes

eolivelli merged commit 9a8b68a into apache:master Oct 11, 2022

This was referenced Jan 13, 2023

[fix][build] Fix "An error has occurred in JaCoCo report generation" #19229

Merged

[fix][ci] Fix codecov reporting by configuring to wait for all builds sending coverage #19237

Merged

[improve][CI] Generate unit test code coverage reports and upload to Codecov #17382

[improve][CI] Generate unit test code coverage reports and upload to Codecov #17382

Conversation

yaalsn commented Sep 1, 2022 • edited Loading

Motivation

Verifying this change

Documentation

Matching PR in forked repository

yaalsn commented Sep 1, 2022

tisonkun commented Sep 1, 2022

yaalsn commented Sep 1, 2022 • edited Loading

nodece left a comment

Choose a reason for hiding this comment

tisonkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoloboschi left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

yaalsn commented Sep 15, 2022

yaalsn commented Sep 15, 2022

tisonkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tisonkun commented Sep 15, 2022

tisonkun commented Sep 15, 2022

codecov-commenter commented Sep 15, 2022 • edited Loading

Codecov Report

yaalsn commented Sep 15, 2022 • edited Loading

yaalsn commented Sep 15, 2022

yaalsn commented Sep 16, 2022

yaalsn commented Sep 18, 2022

yaalsn commented Sep 19, 2022

nodece commented Sep 19, 2022

yaalsn commented Sep 21, 2022

yaalsn commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codelipenghui commented Oct 10, 2022

tisonkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaalsn Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

yaalsn commented Sep 1, 2022 •

edited

Loading

yaalsn commented Sep 1, 2022 •

edited

Loading

codecov-commenter commented Sep 15, 2022 •

edited

Loading

yaalsn commented Sep 15, 2022 •

edited

Loading

yaalsn Oct 10, 2022 •

edited

Loading