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

test: add fuzzers for Builder.Export + inclusion.SubtreeWidth #119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

odeke-em
Copy link
Contributor

In a continuous bid to battle test and secure the network, this change adds 2 fuzzers for:

  • Builder.Export
  • inclusion.SubtreeWidth

@odeke-em odeke-em requested review from a team as code owners October 19, 2024 10:02
@odeke-em odeke-em requested review from cmwaters, ninabarbakadze, Wondertan and renaynay and removed request for a team October 19, 2024 10:02
@celestia-bot celestia-bot requested review from a team October 19, 2024 10:02
@odeke-em odeke-em force-pushed the fuzz-add-FuzzBuilderExport+inclusion.FuzzSubtreeWidth branch 5 times, most recently from f6c0845 to d6f4e64 Compare October 19, 2024 10:26
@rootulp
Copy link
Collaborator

rootulp commented Oct 21, 2024

@odeke-em
Copy link
Contributor Author

@rootulp thanks for the ping but I think that your repo's helpers are giving y'all bad tips and invalid suggestions

  fuzz_test.go:15: File is not `gofumpt`-ed (gofumpt)
33
  	if err := os.MkdirAll(dirPath, 0755); err != nil {

is idiomatic code and the preferred way for quick throw away values in Go. I am going to change it but I highly suggest in the future taking a look at the suggestions.

@odeke-em odeke-em force-pushed the fuzz-add-FuzzBuilderExport+inclusion.FuzzSubtreeWidth branch from 9fe938f to 9c9f5c6 Compare October 25, 2024 09:35
@cristaloleg
Copy link
Contributor

cristaloleg commented Oct 25, 2024

is idiomatic code and the preferred way for quick throw away values in Go. I am going to change it but I highly suggest in the future taking a look at the suggestions.

gofumpt reports 0755 which should be 0o755, not the problem you've mentioned.

@odeke-em
Copy link
Contributor Author

Thank you @cristaloleg! Sadly the CI suggestion did not appear in the error log. Let me update that.

@odeke-em
Copy link
Contributor Author

odeke-em commented Oct 25, 2024

For the record, even that suggestion is pedantic per https://go.dev/play/p/CpKnMA6OAKj but let me push up the suggestion still.

In a continuous bid to battle test and secure the network, this
change adds 2 fuzzers for:
* Builder.Export
* inclusion.SubtreeWidth
@odeke-em odeke-em force-pushed the fuzz-add-FuzzBuilderExport+inclusion.FuzzSubtreeWidth branch from 9c9f5c6 to f2a86d1 Compare October 25, 2024 09:40
@odeke-em
Copy link
Contributor Author

@cristaloleg we have a panic uncovered in 2 minutes of fuzzing per #120. Kindly please help define what happens so that this change can then be merged. Thank you!

@cristaloleg
Copy link
Contributor

Maybe pedantic but definitely error-prone.

@rootulp
Copy link
Collaborator

rootulp commented Nov 13, 2024

@odeke-em do you have a concrete suggestion on what linter settings you would like to be changed for this repo? https://golangci-lint.run/usage/linters/#gofumpt doesn't have a setting to disable just this rule:

Screenshot 2024-11-13 at 4 44 21 PM

so we would need to disable gofumpt entirely which doesn't seem desirable. Additionally, that specific lint rule seems like a good suggestion so I propose we keep gofumpt and modify the code to satisfy the linter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants