[BEAM-14440] Add basic fuzz tests to the coders package #17587
Conversation
|
Can one of the admins verify this patch? |
Codecov Report
@@ Coverage Diff @@
## master #17587 +/- ##
==========================================
- Coverage 73.93% 70.18% -3.75%
==========================================
Files 691 694 +3
Lines 91560 97194 +5634
==========================================
+ Hits 67694 68220 +526
- Misses 22633 27668 +5035
- Partials 1233 1306 +73
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Assigning reviewers. If you would like to opt out of this review, comment R: @damccorm for label go. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Run GoPortable PreCommit |
| @@ -120,3 +120,26 @@ func TestEncodeDecodeStringUTF8LP(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func FuzzEncodeDecodeStringUTF8LP(f *testing.F) { | |||
There was a problem hiding this comment.
Is there a reason to keep the above test that is doing the same thing as this one? Same question for the tests in other files
There was a problem hiding this comment.
We could replace all of the encode/decode tests with fuzz tests that use the testValues as a seed corpus if we wanted to.
There was a problem hiding this comment.
I'd vote we do this rather than duplicating it - it will simplify any future test changes
There was a problem hiding this comment.
Actually I've made a crucial error in that assumption: the fuzz tests ignore any input where the encoding fails, which wouldn't catch a regression in encoding behavior if we replaced the initial encoding tests
There was a problem hiding this comment.
Hm, ok - I'd probably prefer that we split those out into their own test case so we're not double testing the same things, but I don't feel strongly about it - I'll approve and leave that up to you (and/or future reviewers)
There was a problem hiding this comment.
There's an argument for moving the fuzz tests for all of the coders to their own file for clarity
There was a problem hiding this comment.
What is the reasoning behind ignoring encoding failures in the fuzz tests instead of just failing them? If that small change was made there's no reason we couldn't replace the existing tests, right?
There was a problem hiding this comment.
I'm actually curious on this one as well - I'd initially assumed there were inputs that should fail when encoding, but I can't think of any. Are there inputs like that? If so, are there few enough that we can special case them?
| @@ -59,3 +59,23 @@ func TestEncodeDecodeBytes(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func FuzzEncodeDecodeBytes(f *testing.F) { | |||
There was a problem hiding this comment.
Looking at https://go.dev/doc/fuzz/, it looks like these tests won't be run automatically since we're not passing in the fuzz flag, right? We'll just run the seed corpus?
This is neat and probably helps us a bit, but investing heavily in tests that don't get run continuously has a pretty limited ceiling since it can't be used to prove correctness of future changes without taking non-obvious manual steps. We should definitely keep our eyes on this issue to enable continuous fuzzing - until we get there, I'd probably vote we don't make too big of a push towards adding a bunch of fuzz tests. Alternately, we could consider adding our fuzz tests to the set of validations that gets done before a release.
Again - this PR is great and provides a valuable map towards adding fuzz tests, I just want to make sure we're calling out the downsides before going too far down that road.
There was a problem hiding this comment.
The continuous running element is something we can't/don't do right now, the idea would be to eventually add Beam to OSS Fuzz (https://google.github.io/oss-fuzz/) so we have that continuous coverage.
Right now these are mostly proof of concept and relatively simple. I don't anticipate a big push to add fuzz testing everywhere, just targeted coverage where appropriate over time.
There was a problem hiding this comment.
What's the difference between having continuous fuzzing by adding Beam to OSS Fuzz, vs just having a script that runs go test ./... with the fuzz flag and running it as a Jenkins cron job? The latter seems like something we could do immediately as far as I can tell.
There was a problem hiding this comment.
IIRC testing with the fuzz flag is not guaranteed to ever terminate. I tried testing this out and its been running FuzzEncodeDecodeBytes for several minutes.
It also seemed like go test -fuzz ./... didn't actually correctly run the fuzz tests, it seems like they may need to be run one at a time:
dannymccormick-macbookpro:coder dannymccormick$ go test -fuzz ./...
testing: will not fuzz, -fuzz matches more than one fuzz test: [FuzzEncodeDecodeBytes FuzzEncodeDecodeDouble FuzzEncodeDecodeUInt64 FuzzEncodeDecodeInt32 FuzzEncodeDecodeStringUTF8LP]
|
R: @youngoli for final approval |
|
Run GoPortable PreCommit |
2 similar comments
|
Run GoPortable PreCommit |
|
Run GoPortable PreCommit |
Adds a few small fuzz test examples to the coders package, validating that a successful Encode() call will be followed by a successful Decode() call and then have an output matching the input for strings, ints, bytes, and doubles.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.