-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Fix several panics in ParseCalendar #85
Conversation
Thanks. Looks good. Running tests. |
calendar_test.go
Outdated
f.Add(ics) | ||
f.Fuzz(func(t *testing.T, ics []byte) { | ||
_, err := ParseCalendar(bytes.NewReader(ics)) | ||
t.Log(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be t.Errorf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, returning an error is fine, most of the corpus data will be invalid ics. Only a panic is a failure here. I added the log line because I wanted to see the errors I wrote using go test -v
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. I haven't used the fuzzing feature myself. Not nearly enough go coding these days.
Tests are failing for pre-fuzzing go. Perhaps use build tags? Something like:
But that is not correct as it isn't forwards compatible I think. I rarely use go version build constraints |
Hrm.. Should modify the github workflow action to test on more recent versions of go as well. |
I notice you test from 1.14 but the go directive in the mod file is 1.13. |
Cool all tests pass.
Interesting. I didn't notice. That would have happened by accident over time rather than intentional. I don't see a reason to change it right now but will accept a PR/change that pushes the matrix back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Tagged to release for you |
I was trying to Fuzz webcal-proxy but I need golang-ical to be panic free first.