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

Fuzz tests for inclusion and consistency proofs #33

Merged
merged 2 commits into from
Jul 25, 2022

Conversation

hickford
Copy link
Contributor

@hickford hickford commented Jul 18, 2022

Fuzz tests for inclusion and consistency proofs

@hickford hickford force-pushed the morefuzz branch 2 times, most recently from ce4b930 to 9ffe0eb Compare July 18, 2022 16:22
@hickford hickford changed the title More fuzz tests Fuzz tests for inclusion and consistency proofs Jul 18, 2022
@hickford hickford marked this pull request as ready for review July 18, 2022 16:36
@hickford hickford requested a review from a team as a code owner July 18, 2022 16:36
@hickford hickford requested a review from mhutchinson July 18, 2022 16:36
@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2022

Codecov Report

Merging #33 (45167fc) into main (ca422cf) will increase coverage by 0.40%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #33      +/-   ##
==========================================
+ Coverage   88.93%   89.33%   +0.40%     
==========================================
  Files           7        7              
  Lines         497      497              
==========================================
+ Hits          442      444       +2     
+ Misses         50       48       -2     
  Partials        5        5              
Impacted Files Coverage Δ
testonly/tree.go 91.52% <0.00%> (+3.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca422cf...45167fc. Read the comment docs.

@mhutchinson
Copy link
Contributor

What's the motivation behind removing the 1.17 tests? The go.mod file still lists this version as 1.16 compatible. I'm up for bumping this to go1.17 to be consistent with trillian (and soon, trillian-examples: google/trillian-examples#669), but jumping to 1.18 invites using newer language features that may exclude people on older tooling. Not to say we absolutely can't do it, as we don't have an explicit policy, but we've tried to keep support for go versions in the order of a year old thus far.

@AlCutter
Copy link
Collaborator

It's the fuzz testing support - it's new in go1.18 testing lib, so the test would no-longer compile with older releases.

Agreed we should chat about the trade off.

mhutchinson added a commit to mhutchinson/merkle that referenced this pull request Jul 20, 2022
We _may_ want to go further than this to support fuzzing (transparency-dev#33), but as an interim go1.17 is sufficiently old that I don't think we'll be excluding anyone by bumping to this.
@mhutchinson mhutchinson mentioned this pull request Jul 20, 2022
mhutchinson added a commit that referenced this pull request Jul 20, 2022
We _may_ want to go further than this to support fuzzing (#33), but as an interim go1.17 is sufficiently old that I don't think we'll be excluding anyone by bumping to this.
@mhutchinson
Copy link
Contributor

I've just seen #32 and think that this PR with the changes to keep support for 1.17 would make this a super 👍

@hickford hickford marked this pull request as draft July 21, 2022 08:10
Also fuzz HashAt and InclusionProof against reference implementations

Add seed cases to all fuzz tests
@hickford hickford marked this pull request as ready for review July 21, 2022 11:18
testonly/tree_fuzz_test.go Outdated Show resolved Hide resolved
testonly/tree_fuzz_test.go Outdated Show resolved Hide resolved
@mhutchinson
Copy link
Contributor

@AlCutter we could do with making a call on whether the init loops added to this, or the file-based approach in #35 should be our approach to seeding fuzz tests.

@mhutchinson
Copy link
Contributor

@hickford can you take a look at & respond to the open comments on this PR. Once addressed one way or another, I can merge this.

@hickford hickford requested a review from mhutchinson July 25, 2022 12:48
@mhutchinson mhutchinson merged commit 30e96ff into transparency-dev:main Jul 25, 2022
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.

4 participants