Skip to content

[fix][build] Dump Jacoco coverage data to file with JMX interface in TestNG listener#19947

Merged
lhotari merged 2 commits into
apache:masterfrom
lhotari:lh-jacoco-jmx-dump-with-testng-listener
Mar 28, 2023
Merged

[fix][build] Dump Jacoco coverage data to file with JMX interface in TestNG listener#19947
lhotari merged 2 commits into
apache:masterfrom
lhotari:lh-jacoco-jmx-dump-with-testng-listener

Conversation

@lhotari

@lhotari lhotari commented Mar 28, 2023

Copy link
Copy Markdown
Member

Fixes #19931

Motivation

See #19931. Sometimes the default Jacoco shutdown hook doesn't run and there's no coverage data. This causes the Codecov upload to fail since there's no Jacoco coverage report available.

Modifications

  • enable Jacoco agent's JMX interface
  • Dump Jacoco coverage to file with JMX interface in TestNG listener

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: lhotari#145

…TestNG listener

- sometimes the default Jacoco shutdown hook doesn't run and there's no coverage data
@lhotari lhotari force-pushed the lh-jacoco-jmx-dump-with-testng-listener branch from d7a38e5 to 3d25ba9 Compare March 28, 2023 08:16

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems personal CI failed before upload the report - https://github.com/lhotari/pulsar/actions/runs/4540821034/jobs/8002229561?pr=145

@lhotari

lhotari commented Mar 28, 2023

Copy link
Copy Markdown
Member Author

It seems personal CI failed before upload the report - https://github.com/lhotari/pulsar/actions/runs/4540821034/jobs/8002229561?pr=145

@tisonkun it didn't fail because of the changes made in the PR. The building step took 55 minutes and the workflow timed out because of that. I don't see what changes you are requesting.

@tisonkun

Copy link
Copy Markdown
Member

The building step took 55 minutes and the workflow timed out because of that. I don't see what changes you are requesting.

Yes, that's the case. Are your going to rerun the workflow in your fork and notify here once it passed, or we tag this PR as ready-to-test and rerun here?

@lhotari

lhotari commented Mar 28, 2023

Copy link
Copy Markdown
Member Author

Yes, that's the case. Are your going to rerun the workflow in your fork and notify here once it passed, or we tag this PR as ready-to-test and rerun here?

@tisonkun re-running it. in progress at https://github.com/lhotari/pulsar/actions/runs/4540821034/jobs/8006055251?pr=145 . This time build step completed properly. Waiting for the whole build job to complete.

@lhotari

lhotari commented Mar 28, 2023

Copy link
Copy Markdown
Member Author

@tisonkun I revisited the dumping logic once more. It turns out that with maven-surefire-plugin each test is run in it's own TestNG suite. TestNG has a separate listener interface for the completion of all tests.

@lhotari

lhotari commented Mar 28, 2023

Copy link
Copy Markdown
Member Author

/pulsarbot rerun-failure-checks

@lhotari lhotari requested a review from tisonkun March 28, 2023 12:59

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@tisonkun

Copy link
Copy Markdown
Member

Hmmm...Now we can pass the flaky tests job, but it seems the integration tests upload job is still suffering:

image

@lhotari

lhotari commented Mar 28, 2023

Copy link
Copy Markdown
Member Author

Hmmm...Now we can pass the flaky tests job, but it seems the integration tests upload job is still suffering:

this PR doesn't improve upload at all. It's just about dumping the Jacoco exec data.

@codecov-commenter

codecov-commenter commented Mar 28, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.81%. Comparing base (cda2827) to head (c5672e1).
Report is 1764 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19947       +/-   ##
=============================================
+ Coverage     58.92%   72.81%   +13.89%     
- Complexity    25921    31385     +5464     
=============================================
  Files          1846     1859       +13     
  Lines        136607   136808      +201     
  Branches      15033    15047       +14     
=============================================
+ Hits          80498    99623    +19125     
+ Misses        48645    29273    -19372     
- Partials       7464     7912      +448     
Flag Coverage Δ
inttests 24.42% <ø> (?)
systests 25.07% <ø> (?)
unittests 72.09% <ø> (+13.16%) ⬆️

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

see 705 files with indirect coverage changes

@lhotari lhotari merged commit 83f6ea7 into apache:master Mar 28, 2023
@tisonkun

Copy link
Copy Markdown
Member

@lhotari the error info says "not_found" and I read it as file not created. But it seems different from the previous one.

Do we now always dump the file, but the upload action itself is still unstable? IIRC there is an upstream issue for the latter one.

@tisonkun tisonkun mentioned this pull request Mar 29, 2023
2 tasks
@lhotari

lhotari commented Mar 29, 2023

Copy link
Copy Markdown
Member Author

@lhotari the error info says "not_found" and I read it as file not created. But it seems different from the previous one.

Do we now always dump the file, but the upload action itself is still unstable? IIRC there is an upstream issue for the latter one.

Exactly. For the upload issue, it seems that Codecov suggests to not consider the upload token as a secret:

Public repositories that rely on PRs via forks will find that they cannot effectively use Codecov if the token is stored as a GitHub secret. The scope of the Codecov token is only to confirm that the coverage uploaded comes from a specific repository, not to pull down source code or make any code changes.

For this reason, we recommend that teams with public repositories that rely on PRs via forks consider the security ramifications of making the Codecov token available as opposed to being in a secret.

I think we need to ask ASF infra to create the token.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/build doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Please create a coverage file, many PRs failed because of this.

3 participants