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

Make the llvm coverage tests a little more robust. #24624

Closed
wants to merge 2 commits into from

Conversation

c-mita
Copy link
Member

@c-mita c-mita commented Dec 10, 2024

Check that the actual coverage data simply contains the expected coverage data, rather
than checking for an exact match. This matters for when we start to process baseline
coverage, which may result in header files getting included in the report. This is
in-keeping with the other coverage tests.

Also adds the branch data into the reports, which we will check when we're using an LLVM
version that supports branch coverage, and a test that checks header files that include
coverage data are actually included in the final report.

Required for #24593

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Dec 10, 2024
@iancha1992 iancha1992 added the team-Rules-CPP Issues for C++ rules label Dec 10, 2024
Check that the actual coverage data simply contains the expected
coverage data, rather than checking for an exact match. This matters for
when we start to process baseline coverage, which may result in header
files getting included in the report. This is in-keeping with the other
coverage tests.

Also adds the branch data into the reports - this is never disabled when
using LLVM so we should check it.
@c-mita c-mita force-pushed the llvm_cov_test_update branch from 3d9be41 to 4b385b6 Compare December 11, 2024 10:56
@fmeum
Copy link
Collaborator

fmeum commented Dec 11, 2024

Can header files not provide coverage data for regular coverage, e.g. via macros?

@c-mita
Copy link
Member Author

c-mita commented Dec 11, 2024

Can header files not provide coverage data for regular coverage, e.g. via macros?

There are cases where header files should produce coverage (e.g. templates), just not in these test cases.

However, I seem to remember having issues with LLVM and coverage for header files that results in them either not being generated or not ending up in the final report.

@c-mita
Copy link
Member Author

c-mita commented Dec 11, 2024

I've added a test to check that we do get coverage for header files that actually contribute something.

It seems that LLVM omits files that contribute nothing (headers that only contain declarations), which conflicts somewhat with our current baseline coverage implementation.
Down the road, when we can actually generate a baseline coverage file more intelligently, this probably won't be a problem.

@c-mita c-mita added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 11, 2024
@c-mita
Copy link
Member Author

c-mita commented Dec 11, 2024

I'll be doing the import myself.

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants