Skip to content

Simplify fuzzing coverage (CI) scripts somewhat #3925

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

Merged
merged 3 commits into from
Jul 21, 2025

Conversation

TheBlueMatt
Copy link
Collaborator

We recently introduced uploading coverage from no-corpus fuzzing runs into codecov in CI. Here, we simplify some of the scripts that do so, especially removing the second ci-fuzz.sh file we added to the repo.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jul 14, 2025

👋 Thanks for assigning @joostjager 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.

Comment on lines 134 to 138
./contrib/generate_fuzz_coverage.sh --output-dir `pwd`
# Could you use this to fake the coverage report for your PR? Sure.
# Will anyone be impressed by your amazing coverage? No
# Maybe if codecov wasn't broken we wouldn't need to do this...
bash <(curl -s https://codecov.io/bash) -f fuzz-codecov.json -t "f421b687-4dc2-4387-ac3d-dc3b2528af57"
Copy link

Choose a reason for hiding this comment

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

There appears to be a path mismatch in the coverage file handling. The script is called with --output-dir \pwd`which will create the coverage file at`pwd`/fuzz-codecov.json, but the codecov upload command is looking for fuzz-codecov.json` in the current directory without a path prefix.

To fix this, either:

  1. Add the --output-codecov-json flag to the script call, or
  2. Update the file path in the bash command to \pwd`/fuzz-codecov.json`

This will ensure the codecov upload can find the generated coverage file.

Suggested change
./contrib/generate_fuzz_coverage.sh --output-dir `pwd`
# Could you use this to fake the coverage report for your PR? Sure.
# Will anyone be impressed by your amazing coverage? No
# Maybe if codecov wasn't broken we wouldn't need to do this...
bash <(curl -s https://codecov.io/bash) -f fuzz-codecov.json -t "f421b687-4dc2-4387-ac3d-dc3b2528af57"
./contrib/generate_fuzz_coverage.sh --output-dir `pwd`
# Could you use this to fake the coverage report for your PR? Sure.
# Will anyone be impressed by your amazing coverage? No
# Maybe if codecov wasn't broken we wouldn't need to do this...
bash <(curl -s https://codecov.io/bash) -f `pwd`/fuzz-codecov.json -t "f421b687-4dc2-4387-ac3d-dc3b2528af57"

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

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

Small PR, but still difficult to review without much explanation. Commit message just says 'simplify', but adding self-comments in the PR explaining why this is all equivalent would be helpful.

@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.

--output-codecov-json)
OUTPUT_CODECOV_JSON=1
shift 1
;;
*)
echo "Unknown option: $1"
echo "Usage: $0 [--output-dir OUTPUT_DIRECTORY]"
Copy link

Choose a reason for hiding this comment

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

The usage information is missing the newly added --output-codecov-json flag. Please update the help text to include this option:

echo "Usage: $0 [--output-dir OUTPUT_DIRECTORY] [--output-codecov-json]"

This will help users understand all available command-line options.

Suggested change
echo "Usage: $0 [--output-dir OUTPUT_DIRECTORY]"
echo "Usage: $0 [--output-dir OUTPUT_DIRECTORY] [--output-codecov-json]"

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its a CI-only flag so not really worth documenting.

Copy link

codecov bot commented Jul 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.02%. Comparing base (b939100) to head (37cd32d).
Report is 87 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##             main    #3925     +/-   ##
=========================================
  Coverage   89.01%   89.02%             
=========================================
  Files         166      167      +1     
  Lines      119692   121800   +2108     
  Branches   119692   121800   +2108     
=========================================
+ Hits       106546   108427   +1881     
- Misses      10740    10956    +216     
- Partials     2406     2417     +11     
Flag Coverage Δ
fuzz ?
fuzzing 22.73% <ø> (?)
tests 88.84% <ø> (?)

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.

@TheBlueMatt TheBlueMatt force-pushed the 2025-07-3718-followups branch from 95df8a0 to 427d984 Compare July 16, 2025 17:30
We recently introduced uploading coverage from no-corpus fuzzing
runs into codecov in CI. Here, we simplify some of the scripts that
do so, especially removing the second `ci-fuzz.sh` file we added to
the repo.
codecov recommends using the new CLI uploader rather than their
(deprecated) bash uploader (which we use). It also appears to have
a fail-on-error mode which we'd prefer over the bash one which
silently swallows errors.
@TheBlueMatt TheBlueMatt force-pushed the 2025-07-3718-followups branch from 427d984 to 08795f2 Compare July 17, 2025 19:47
@TheBlueMatt
Copy link
Collaborator Author

Okay, this seems to work, and codecov's report includes coverage for lines that are #[cfg(fuzzing)].

@TheBlueMatt TheBlueMatt requested a review from joostjager July 18, 2025 12:12
Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

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

If I understand it correctly, all the coverage now ends up in the same report?

Its useful to be able to identify what kind of coverage we have for
a line - test coverage is very different in nature from fuzzing
coverage. Here we pass separate "job names" to codecov uploads to
do so.
@TheBlueMatt TheBlueMatt force-pushed the 2025-07-3718-followups branch from 059c0cf to 37cd32d Compare July 18, 2025 22:17
Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

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

One unclarified issue around fuzz coverage (process_onion_failure), but seems unlikely to have anything to do with this PR.

@TheBlueMatt
Copy link
Collaborator Author

Landing as CI-only.

@TheBlueMatt TheBlueMatt merged commit eac01d0 into lightningdevkit:main Jul 21, 2025
28 checks passed
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.

3 participants