Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat][ci] Collect code coverage for integration tests from docker containers #19263

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jan 17, 2023

Motivation

Code coverage metrics don't currently consider integration tests and systems tests that are run as part of the Pulsar CI pipeline. There hasn't been a way to measure the total code coverage of Pulsar CI pipeline tests although it would be useful to see what parts of Pulsar code base are tested and what aren't.

Modifications

  • Add solution to Pulsar Testcontainer based integration tests so that the Jacoco agent gets activated for processes that are run inside Docker containers
    • A directory from the host is mounted in the docker container. This directory is used for storing the Jacoco exec files.
  • The code coverage should be uploaded to codecov for all public code repositories. This is now changed so that it's possible to get coverage metrics for builds running in a fork.
  • Graceful shutdown of Java processes is needed for collecting Jacoco exec data since by default, the Jacoco agent dumps the exec data to a file in a shutdown hook.
    • For processes managed with supervisor, it's necessary to run "supervisorctl stop all" to trigger a graceful shutdown
      • It was necessary to stop Zookeeper last for preventing shutdown getting stuck for Brokers or Bookies.
  • Creating the Jacoco XML report needed for Codecov is created using Jacoco command line tools.
    • The script has been added as part of build/run_integration_group.sh script.
      • The jar files of the Docker image is needed to create the XML report from the exec files.

Documentation

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

Matching PR in forked repository

PR in forked repository: lhotari#134

@lhotari lhotari added this to the 2.12.0 milestone Jan 17, 2023
@lhotari lhotari self-assigned this Jan 17, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 17, 2023
@lhotari lhotari changed the title [feat][ci] Collect code coverage for integration tests [feat][ci] Collect code coverage for integration tests from docker containers Jan 17, 2023
@lhotari
Copy link
Member Author

lhotari commented Jan 17, 2023

@yaalsn Please review this PR.

@lhotari lhotari force-pushed the lh-collect-int-test-code-coverage branch from d69da8f to d0948cc Compare January 17, 2023 16:40
@lhotari lhotari force-pushed the lh-collect-int-test-code-coverage branch from d0948cc to 23c1865 Compare January 17, 2023 17:28
@lhotari
Copy link
Member Author

lhotari commented Jan 17, 2023

Example of code coverage results for an earlier experiment: https://app.codecov.io/github/lhotari/pulsar/commit/369dec198b5cf6b98cd5c52a0c32126974a8e9ac/tree

@lhotari
Copy link
Member Author

lhotari commented Jan 17, 2023

Codecov results of the currently running PR build in the fork will be available at https://app.codecov.io/github/lhotari/pulsar/commit/23c1865976414a166e4ff2d6c5874937390cfc9e/tree

@lhotari
Copy link
Member Author

lhotari commented Jan 17, 2023

#19264 will also be needed to fix coverage results.

Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

This is awesome!

@lhotari
Copy link
Member Author

lhotari commented Jan 17, 2023

/pulsarbot rerun-failure-checks

@aymkhalil
Copy link
Contributor

I wonder if including integration tests in the code coverage report would hide gaps in unit test coverage (unless the latter coverage metric was already broken :/)

Is there a possibility to separate the coverage report for unit vs integ tests?

@lhotari
Copy link
Member Author

lhotari commented Jan 17, 2023

I wonder if including integration tests in the code coverage report would hide gaps in unit test coverage (unless the latter coverage metric was already broken :/)

Is there a possibility to separate the coverage report for unit vs integ tests?

Yes, it should be possible to view unit test coverage separately. They are reported to Codecov with different flags.

@lhotari
Copy link
Member Author

lhotari commented Jan 17, 2023

@aymkhalil The docs for the flags feature in Codecov: https://docs.codecov.com/docs/flags . It seems that the full feature is disabled by default.

@lhotari
Copy link
Member Author

lhotari commented Jan 17, 2023

To experiment, I enabled it for my fork at https://app.codecov.io/github/lhotari/pulsar/flags . For apache/pulsar , I don't have access so I'll have to create a ASF ticket for handling that.

@lhotari lhotari merged commit 9825b59 into apache:master Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants