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.

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

Jump to bottom

Add coverage support to C++ regression tests #212

Merged
merged 1 commit into from
Dec 2, 2022
Merged

Add coverage support to C++ regression tests #212

merged 1 commit into from
Dec 2, 2022

Conversation

fmeum
Copy link
Member

@fmeum fmeum commented Sep 28, 2022 •
edited
Loading

Co-authored-by: Zhen Yu Ding zhendeveloper@gmail.com

Closes #174

@fmeum fmeum marked this pull request as ready for review September 28, 2022 10:15
@fmeum fmeum changed the title Add coverage support to regression tests Add coverage support to C++ regression tests Sep 28, 2022
@fmeum
Copy link
Member Author

fmeum commented Sep 28, 2022 •
edited
Loading

@stefanbucur Could you review this? This differs from #174 in that the LCOV merger is no longer built without bazel coverage.

@zhenyudg I added you as a coauthor.

@zhenyudg zhenyudg mentioned this pull request Oct 6, 2022
Closed
@fmeum
Copy link
Member Author

fmeum commented Nov 10, 2022

@stefanbucur Friendly ping

@stefanbucur
Copy link
Collaborator

stefanbucur commented Nov 10, 2022 •
edited
Loading

Thanks so much for the ping, it totally fell through the cracks. I'll take a look by EOD.

(PS: In general, please feel free to ping me if you don't get a review within a few days. Nowadays I'm spending less time on Github so things may easily slip off my attention.)

stefanbucur
stefanbucur requested changes Nov 11, 2022
View reviewed changes
Copy link
Collaborator

@stefanbucur stefanbucur left a comment

Choose a reason for hiding this comment

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

This is great - thanks so much for figuring out a way to do this cleanly. Just small-ish comments / suggestions.

fuzzing/tools/noop_lcov_merger.sh Show resolved Hide resolved
fuzzing/tools/BUILD Outdated
)

config_setting(
name = "collect_code_coverage",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Maybe use a different name to avoid confusion with the real flag at use sites? E.g. coverage_collection_enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

stefanbucur
stefanbucur approved these changes Nov 14, 2022
View reviewed changes
Copy link
Collaborator

@stefanbucur stefanbucur left a comment

Choose a reason for hiding this comment

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

LGTM, but we seem to have new test failures?

@fmeum
Copy link
Member Author

fmeum commented Nov 14, 2022 •
edited
Loading

LGTM, but we seem to have new test failures?

Looks like Bazel CI OSS-Fuzz replaced python with python3, which is incompatible with Bazel 4. I will try raising the minimum version and see whether that fixes the failure.

@fmeum
Copy link
Member Author

fmeum commented Nov 15, 2022 •
edited
Loading

@stefanbucur I updated the Bazel version to 5.0.0, which makes Bazel look for python3 in PATH. The CI run still fails, but when I run /usr/bin/env python3 in base-builder locally, that passes. Not sure what's wrong here.

@stefanbucur
Copy link
Collaborator

@stefanbucur I updated the Bazel version to 5.0.0, which makes Bazel look for python3 in PATH. The CI run still fails, but when I run /usr/bin/env python3 in base-builder locally, that passes. Not sure what's wrong here.

Interesting indeed 🤔 FWIW, the CI fuzzing test has been broken for about 3 weeks at head too: https://github.com/bazelbuild/rules_fuzzing/actions/workflows/oss_fuzz.yml

@fmeum fmeum mentioned this pull request Nov 21, 2022
Merged
oliverchang pushed a commit to google/oss-fuzz that referenced this pull request Nov 22, 2022
@fmeum
bazel-rules-fuzzing-test: Link python to python3 (#9024)
f330408 
rules_fuzzing uses Bazel 4, which still expects `python` in `PATH`.

Work towards
bazel-contrib/rules_fuzzing#212 (comment)
@fmeum fmeum force-pushed the coverage branch 2 times, most recently from 8638b35 to 1d17ea8 Compare December 2, 2022 07:53
@fmeum @zhenyudg
Add coverage support to C++ regression tests
a3640d4 
Co-authored-by: Zhen Yu Ding <zhendeveloper@gmail.com>
@fmeum
Copy link
Member Author

fmeum commented Dec 2, 2022

@stefanbucur Tests are passing now.

@stefanbucur stefanbucur merged commit 040489e into bazel-contrib:master Dec 2, 2022
@fmeum fmeum deleted the coverage branch December 2, 2022 20:23
eamonnmcmanus pushed a commit to eamonnmcmanus/oss-fuzz that referenced this pull request Mar 15, 2023
@fmeum @eamonnmcmanus
bazel-rules-fuzzing-test: Link python to python3 (google#9024)
4b42de1 
rules_fuzzing uses Bazel 4, which still expects `python` in `PATH`.

Work towards
bazel-contrib/rules_fuzzing#212 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanbucur stefanbucur stefanbucur approved these changes

@asraa asraa Awaiting requested review from asraa asraa is a code owner

@inferno-chromium inferno-chromium Awaiting requested review from inferno-chromium inferno-chromium is a code owner

@fniksic fniksic Awaiting requested review from fniksic fniksic is a code owner

@oliverchang oliverchang Awaiting requested review from oliverchang oliverchang is a code owner

@xinhaoyuan xinhaoyuan Awaiting requested review from xinhaoyuan xinhaoyuan is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fmeum @stefanbucur