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

GitHub action for code coverage #12

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Conversation

SuhashiniNaik
Copy link
Contributor

@SuhashiniNaik SuhashiniNaik commented Nov 6, 2024

This GitHub Action automates the generation and publishing of code coverage reports of the modules. It captures code coverage data using lcov, filters out third-party, mock files, gtest folders and system libraries.
Then it generates an HTML report that is deployed to GitHub Pages.

Changes made:
Added code-coverage.yml
Added CodeCoverage.cmake
Made changes in CMakeLists.txt

Comment on lines 44 to 49
- name: Filter out 3rd party and mock files
run: |
echo "Filtering out 3rd party and mock files from coverage data..."
lcov --remove cmake-build-unit-tests/coverage_unfiltered.info \
'*libs/3rdparty/googletest/*' \
'*/mock/*' \
'*/gmock/*' \
'*/usr/include/*' \
'*/gtest/*' \
--output-file cmake-build-unit-tests/coverage.info
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than exclude a whole list of locations, only exclude the mock/test patterns and instead use options like:

	•	--directory: Specifies the directory containing .gcda and .gcno files.
	•	--capture: Captures coverage data.
	•	--include/--extract: Includes files matching specific patterns.
	•	--remove: Excludes files matching specific patterns.
	•	--no-external: Excludes coverage data for files outside the specified directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes Oliver, I have excluded the mock/test patterns but I also had to exclude 3rd party files because they were not getting excluded by using --no-external command in the latest commit. It works for excluding system libraries though

Comment on lines 27 to 28
- name: Test
# Execute tests defined by the CMake configuration.
run: |
cmake -B cmake-build-unit-tests -S executables/unitTest -DBUILD_UNIT_TESTS=ON
cmake --build cmake-build-unit-tests -j4
ctest --test-dir cmake-build-unit-tests -j4
Copy link
Contributor

Choose a reason for hiding this comment

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

since we run the tests in this action, we should remove the test execution in the build.yml file

@ThoFrank
Copy link
Contributor

ThoFrank commented Nov 7, 2024

I think this action should be split into parts. Similar to the documentation action:

  1. Build the coverage, zip it and upload the artifact. Should only run on PR's. This way you can still view the output by downloading the zip.
  2. Download the zip, unpack it and publish it. Should only run on push to master (and as a dependency to action 1).

Action 2 could be combined with the doc publish action.

Additionally action 1 could also be integrated into the release workflow. But probably this should be done in another PR, because the permissions in the release workflow need to be fixed.

@marcmo
Copy link
Contributor

marcmo commented Nov 8, 2024

@ThoFrank that change makes sense...we only want to re-publish the coverage results when a commit is done to the main branch. where is the zip file stored and how can contributors access it?

@SuhashiniNaik Please also squash your commits to the feature branch.

@ThoFrank
Copy link
Contributor

ThoFrank commented Nov 8, 2024

@marcmo Right now the link is only available in the action log. But we could look into posting the link as a comment to the pr.

For an example see here -> Upload docs

@SuhashiniNaik SuhashiniNaik force-pushed the coverage branch 2 times, most recently from 0dcafad to 5abf6d1 Compare November 12, 2024 11:20
@ThoFrank ThoFrank mentioned this pull request Nov 12, 2024
@SuhashiniNaik SuhashiniNaik force-pushed the coverage branch 2 times, most recently from 52d3e7d to 0217983 Compare November 13, 2024 05:26
.github/workflows/build.yml Outdated Show resolved Hide resolved
@SuhashiniNaik SuhashiniNaik marked this pull request as draft November 15, 2024 11:50
@SuhashiniNaik SuhashiniNaik force-pushed the coverage branch 4 times, most recently from d50761d to cd34715 Compare November 18, 2024 08:30
@vladyslavmarkovaccenture
Copy link
Contributor

Since we don't have debug symbols in the debug build now the code coverage info might be imprecise. This is fixed in #23 along with failing unit test. I suggest merging it first.

@SuhashiniNaik SuhashiniNaik force-pushed the coverage branch 3 times, most recently from 0c9e3f6 to 71d68d9 Compare November 20, 2024 06:18
@SuhashiniNaik SuhashiniNaik requested a review from marcmo November 20, 2024 06:24
@SuhashiniNaik SuhashiniNaik marked this pull request as ready for review November 20, 2024 06:33
@SuhashiniNaik SuhashiniNaik force-pushed the coverage branch 2 times, most recently from edd6f30 to 9489c9e Compare November 25, 2024 13:09
Copy link
Contributor

@marcmo marcmo left a comment

Choose a reason for hiding this comment

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

LGTM

Add code-coverage Github action

Add code-coverage GitHub action

Add code-coverage GitHub action
@SuhashiniNaik SuhashiniNaik requested a review from marcmo November 26, 2024 05:53
@marcmo marcmo merged commit fe12a26 into eclipse-openbsw:main Nov 26, 2024
1 check passed
@marcmo marcmo deleted the coverage branch November 26, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants