Skip to content

ci: enable coverage for pull requests #1065

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 13, 2025
+26 −31
Merged

ci: enable coverage for pull requests #1065

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 26 additions & 31 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,17 +515,11 @@ jobs:
github_token: ${{ secrets.GITHUB_TOKEN }}

- name: Codecov
id: codecov
if: matrix.coverage
env:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
run: |
set -x

if [ -z "$CODECOV_TOKEN" ]; then
echo "CODECOV_TOKEN is not set. Skipping coverage report."
exit 0
fi

set -euvx

# Find gcov
gcov_tool="gcov"
for version in "${{steps.setup-cpp.outputs.version-major}}.${{steps.setup-cpp.outputs.version-minor}}" "${{steps.setup-cpp.outputs.version-major}}"; do
Expand All @@ -534,28 +528,29 @@ jobs:
break
fi
done

for dir in "./build"; do
# Generate reports
echo "Generate report: $dir"
lcov --rc lcov_branch_coverage=0 --gcov-tool "$gcov_tool" --directory "$dir" --capture --output-file "$dir/all.info" --ignore-errors inconsistent
lcov --rc lcov_branch_coverage=0 --ignore-errors inconsistent --list "$dir/all.info"

# Upload to codecov
echo "Upload to codecov: $dir"
bash <(curl -s https://codecov.io/bash) -f "$dir/all.info"
done

# Summary
echo "# Coverage" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "[![codecov](https://codecov.io/github/$GITHUB_REPOSITORY/commit/$GITHUB_SHA/graphs/sunburst.svg)](https://codecov.io/github/$GITHUB_REPOSITORY/commit/$GITHUB_SHA)" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "Commit: [![codecov](https://codecov.io/github/$GITHUB_REPOSITORY/commit/$GITHUB_SHA/graph/badge.svg)](https://codecov.io/github/$GITHUB_REPOSITORY/commit/$GITHUB_SHA)" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "Branch: [![codecov](https://codecov.io/github/$GITHUB_REPOSITORY/branch/$GITHUB_REF_NAME/graph/badge.svg)](https://codecov.io/github/$GITHUB_REPOSITORY/commit/$GITHUB_SHA)" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY

dir="./build"
lcov --rc lcov_branch_coverage=0 --gcov-tool "$gcov_tool" --directory "$dir" --capture --output-file "$dir/all.info" --ignore-errors inconsistent
lcov --rc lcov_branch_coverage=0 --ignore-errors inconsistent --list "$dir/all.info"
echo "file=$(realpath "$dir/all.info")" >> $GITHUB_OUTPUT

- name: Upload Coverage as Artifact
if: matrix.coverage
uses: actions/upload-artifact@v4
with:
name: Coverage
path: ${{ steps.codecov.outputs.file }}
retention-days: 30

- name: Codecov Upload
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks great.

This action already embeds that feature that fails on reduced coverage, right?

Should all PRs be blocked by that? I know most should. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am testing how that behaves, but I think if we need to fine tune that behavior, we can leave it to another PR and have this one be just about having coverage reports on PRs.

I think it's reasonable for decreased coverage to block, if it can still be overridden intentionally.

We may need to adjust the logic about how it counts decreased coverage.

It can't be about decreased percentage covered, because that can block NFC PRs which reduce amount of covered lines.
I think the logic should be about blocking when adding uncovered lines.

There are several options to choose here, and this needs some study and experiments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea. The GitHub action may have a reasonable threshold. In parallel, I should check if there's a way for GitHub to allow merging PRs that fail CI under exceptional circumstances.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can see here it does fail the PR: #1066

But it can still be merged without any hassle.

I think this can be improved through GitHub configuration changes.

The LLVM Repo has a setting where there is a check box you have to mark saying you want to "bypass the rules", so it looks less prone to accidents.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can see here it does fail the PR: #1066

That's valuable information. Now I’m the one having trouble keeping my side of the promise:

Once I set up the repository so that no merge could be done unless everything passed CI. My plan now was to tweak something there to handle the coverage issue, because we know that from time to time this test gives us false negatives (or positives, depending on how you define it) and blocks the merge.

I can’t seem to find where that configuration is anymore. Maybe something changed and that setting doesn’t even exist now, since you already tried it and it seems to let you merge. But I’d still like other cases to block the PR.

Do you have any suggestion for how to achieve that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you have to go into the branch settings in the GitHub settings page for this repo, and enable this:
image

That should probably be done for the master and develop branches.

Then I believe you have to set up so every developer role has the bypass branch protection permission.

Then maybe only allow bypassing the rules on the develop branch, but not on master. Should be this setting on the same page:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh... Yes. That's the setting we had. So someone must have just removed it altogether without saying anything. 😐😞😩

uses: codecov/codecov-action@v5
if: matrix.coverage
with:
fail_ci_if_error: true
files: ${{ steps.codecov.outputs.file }}
disable_search: true
token: ${{ secrets.CODECOV_TOKEN }}
verbose: true


releases:
Expand Down
Loading