Skip to content

Conversation

@Anyitechs
Copy link
Contributor

Following the work (#3718 and #3925) that introduced uploading coverage from no-corpus fuzzing runs into codecov in CI. This PR focuses on uploading the CI-generated fuzz corpus coverage into codecov in CI.

Closes #3926

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Oct 10, 2025

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@codecov
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.17%. Comparing base (fdc8731) to head (6cd3f8f).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4153      +/-   ##
==========================================
+ Coverage   88.82%   89.17%   +0.34%     
==========================================
  Files         180      180              
  Lines      137278   137271       -7     
  Branches   137278   137271       -7     
==========================================
+ Hits       121944   122406     +462     
+ Misses      12522    12257     -265     
+ Partials     2812     2608     -204     
Flag Coverage Δ
fuzzing 31.17% <ø> (+9.68%) ⬆️
tests 88.64% <ø> (-0.03%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Anyitechs Anyitechs marked this pull request as draft October 10, 2025 18:42
@Anyitechs Anyitechs force-pushed the upload-fuzz-coverage branch from dc493c2 to fdf6799 Compare October 13, 2025 00:25
@Anyitechs Anyitechs marked this pull request as ready for review October 13, 2025 01:52
@tankyleo tankyleo requested review from TheBlueMatt and removed request for tankyleo October 13, 2025 22:16
for target_dir in hfuzz_workspace/*; do
[ -d "$target_dir" ] || continue
src_name="$(basename "$target_dir")"
for dest in "$src_name" "${src_name%_target}"; do
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to copy into $src_name.

mkdir -p "test_cases/$dest"
# Copy corpus files into the test_cases directory
find "$target_dir" -maxdepth 2 -type f \
\( -path "$target_dir/CORPUS/*" -o -path "$target_dir/INPUT/*" -o -path "$target_dir/NEW/*" -o -path "$target_dir/input/*" \) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we're just looking in hfuzz_workspace, I believe we only need to look in input, not CORPUS, INPUT, or NEW.

cargo clean
- name: Run fuzzers
run: cd fuzz && ./ci-fuzz.sh && cd ..
- name: Upload honggfuzz corpus
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than only uploading, is there a way to make this directory persistent so that we can keep it between fuzz jobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if we really need to persist the directory here. My understanding is that the fuzz job runs on the latest code changes on every PR, so the generated corpus is tailored to the code changes on that PR. If we persist the corpus from a previous run and use that on a new run, won't that produce incorrect/misleading coverage data?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the point of the fuzz job is only to generate coverage data, but rather test the code :). Having a bit more coverage data from fuzzing than we "deserve" is okay, at least now that we split the coverage data out so that codecov shows fuzzing separately, and having persistent fuzzing corpus means our fuzzing is much more likely to catch issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, how long do you think we can have this directory persisted? The upload-artifact action have a retention-days input that can be used to persist the artifact for a while. The default is 90 days but can be adjusted (https://github.com/actions/upload-artifact?tab=readme-ov-file#retention-period).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the simple "upload-artifact" task just stores data for this CI run. What I was thinking is some kind of persistent directory that's shared across jobs so that each CI fuzz task picks up the latest directory, does some fuzzing, finds new test cases, then uploads a new copy with more tests in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I was thinking is some kind of persistent directory that's shared across jobs so that each CI fuzz task picks up the latest directory, does some fuzzing, finds new test cases, then uploads a new copy with more tests in it.

Makes sense. I pushed eea2e4b to handle this using Github's cache action (https://github.com/actions/cache?tab=readme-ov-file).

# Copy corpus files into the test_cases directory
find "$target_dir" -maxdepth 2 -type f \
\( -path "$target_dir/CORPUS/*" -o -path "$target_dir/INPUT/*" -o -path "$target_dir/NEW/*" -o -path "$target_dir/input/*" \) \
-print0 | xargs -0 -I{} cp -n {} "test_cases/$dest/" 2>/dev/null || true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
-print0 | xargs -0 -I{} cp -n {} "test_cases/$dest/" 2>/dev/null || true
-print0 | xargs -0 -I{} cp -n {} "test_cases/$dest/"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thank you.

done
# Check if any files were actually imported
if [ -n "$(find test_cases -type f -print -quit 2>/dev/null)" ]; then
imported=1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure its worth the extra effort just to print differently.

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@Anyitechs
Copy link
Contributor Author

Thank you for the review.

I've addressed all feedbacks and pushed a fixup here 1e4a7c5

@Anyitechs Anyitechs requested a review from TheBlueMatt October 23, 2025 12:07
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Responded at #4153 (comment)

@Anyitechs Anyitechs force-pushed the upload-fuzz-coverage branch 2 times, most recently from 19c1495 to eea2e4b Compare October 27, 2025 16:17
@Anyitechs
Copy link
Contributor Author

Anyitechs commented Oct 27, 2025

I rebased on main and that pulled in a dependency update to proptest 1.9.0 which has broken the 1.75.0 MSRV check. This seems unrelated to my changes, but CI is failing because of that.

EDIT: This seems to be blocking the build (and fuzzing as well).

@Anyitechs Anyitechs requested a review from TheBlueMatt October 27, 2025 16:42
We have some complexity in `ci-fuzz.sh` to limit each fuzzer to a
rough runtime, but `honggfuzz` has a `--run-time` argument that we
can simply use instead, which we do here.
This now slows us down as we run our fuzz job on a machine with
more than one or two cores.
@TheBlueMatt
Copy link
Collaborator

Yea, sorry, CI is kinda a mess for three reasons all at once. Can you rebase on #4179? That should get at least the fuzz job running again, even if not others.

TheBlueMatt and others added 3 commits October 28, 2025 02:01
honggfuzz builds appear to fail on 1.75 - 1.79, so we just pick an
MSRV of the first toolchain that actually works.

Sadly, github for some reason drops a trailing in the env entry, so
we copy the rust version a few places.
Because each CI job runs on a fresh runner and can't share data between jobs. We rely on Github Actions upload-artifact and download-artifact to share the CI generated fuzz corpus, then replay them in the `contrib/generate_fuzz_coverage.sh` script to generate the coverage report.
@Anyitechs Anyitechs force-pushed the upload-fuzz-coverage branch from eea2e4b to de9e1fd Compare October 28, 2025 08:55
@Anyitechs
Copy link
Contributor Author

Can you rebase on #4179? That should get at least the fuzz job running again, even if not others.

Yes, done! Thank you.

@tnull
Copy link
Contributor

tnull commented Oct 28, 2025

FWIW, remaining CI failures should be resolved shortly by #4180

uses: actions/cache@v4
with:
path: fuzz/hfuzz_workspace
key: fuzz-corpus-${{ github.ref }}-${{ github.sha }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this going to be per-pr? We don't want it to be per-pr we want it to be global.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this going to be per-pr? We don't want it to be per-pr we want it to be global.

Addressed this by adding a two-step logic to the workflow: a read-only per-pr step that seeds the fuzzer for a more effective run on PRs and a main branch step that does same but also writes to the global cache.

@Anyitechs Anyitechs force-pushed the upload-fuzz-coverage branch from de9e1fd to fcba095 Compare October 28, 2025 16:46
@Anyitechs Anyitechs force-pushed the upload-fuzz-coverage branch from fcba095 to 6cd3f8f Compare October 28, 2025 17:10
@Anyitechs Anyitechs requested a review from TheBlueMatt October 28, 2025 19:24
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.

Upload CI-generated fuzz corpus coverage to codecov

4 participants