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

feat(kcov): add code coverage support for tests + fuzzers #193

Merged
merged 32 commits into from
Jul 18, 2024
Merged

Conversation

0xNineteen
Copy link
Contributor

@0xNineteen 0xNineteen commented Jul 10, 2024

  • adds support for kcov (can be used in both unit tests and fuzzing)
  • adds new fuzz docs on how to use it in docs/fuzzing.md
  • updates gossip fuzzing code to support fuzzing the gossipService in the same process
    • prev we would only support fuzzing across the network
  • see scripts/kcov_test.sh for how to run kcov on tests

the goal is to use code coverage to improve pr reviews and understand where things are and aren't being tested - not as a metric to optimize to 100% coverage

TODO: move sh scripts to the zig system

@0xNineteen 0xNineteen marked this pull request as ready for review July 10, 2024 18:16
@0xNineteen 0xNineteen self-assigned this Jul 10, 2024
@0xNineteen
Copy link
Contributor Author

example coverage run

https://github.com/Syndica/sig/actions/runs/9896446984/job/27338682812?pr=193

…#195)

* running kcov on the unit tests lead to finding important code paths we werent testing - this pr adds these units tests - this includes tests for both gossip and accounts-db
* add fuzz testing for the gossip table
@0xNineteen
Copy link
Contributor Author

0xNineteen commented Jul 15, 2024

note: the two prs merged (in the last two commits) were built off of this main pr - so i merged them to make review easier

the summary of the two prs include:

  • adding unit tests for code paths we werent testing (discovered using kcov)
  • adding fuzz testing to the gossip table (and using kcov too)

Copy link
Contributor

@InKryption InKryption left a comment

Choose a reason for hiding this comment

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

A few style and correctness suggestions.

src/gossip/fuzz_service.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_service.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_table.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_table.zig Outdated Show resolved Hide resolved
src/accountsdb/fuzz.zig Outdated Show resolved Hide resolved
src/fuzz.zig Outdated Show resolved Hide resolved
src/gossip/data.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_table.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_table.zig Outdated Show resolved Hide resolved
Copy link
Contributor

@InKryption InKryption left a comment

Choose a reason for hiding this comment

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

One final small review & inquiries of some stylistic & correctness details. After this pass-over I'm happy.
Also, I think it would be good to dedicate a separate PR for porting the shell scripts to zig, to avoid delaying the merge of the rest of these already extremely valuable changes due to potential hiccups in that process.

src/accountsdb/db.zig Outdated Show resolved Hide resolved
src/accountsdb/index.zig Outdated Show resolved Hide resolved
src/accountsdb/index.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_service.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_service.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_table.zig Outdated Show resolved Hide resolved
Copy link
Contributor

@yewman yewman left a comment

Choose a reason for hiding this comment

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

Couple of instances where I think it would be nicer to use durations rather than raw ints, or if using raw ints ensure that the units are clear from the value name, other than that looks good to me.

src/gossip/fuzz_service.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_table.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_table.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_table.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_service.zig Outdated Show resolved Hide resolved
src/gossip/fuzz_service.zig Outdated Show resolved Hide resolved
@InKryption InKryption dismissed their stale review July 17, 2024 20:08

Everything has been addressed

@0xNineteen 0xNineteen merged commit e05ca73 into main Jul 18, 2024
6 checks passed
@0xNineteen 0xNineteen deleted the 19/kcov branch July 18, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants