Straightforwardly fixes a few minor copy bugs and adds a small fuzz util #5572
Conversation
davidporter-id-au
requested review from
Shaddoll,
neil-xie,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| result.Input = make([]byte, len(sourceInfo.Input)) | ||
| copy(result.Input, sourceInfo.Input) | ||
| } | ||
| if sourceInfo.Control != nil { |
There was a problem hiding this comment.
I checked the Control field a few weeks ago to see why/where it is used and couldn't find anything explanatory. Do you know why we carry over this Control field?
There was a problem hiding this comment.
I have zero idea
There was a problem hiding this comment.
I tried to figure it out a while ago too :|
I think the answer is somewhere in git history, but I didn't find anything understandable.
| // skips the majority of difficult to generate values | ||
| // for the sake of simplicity in testing. Use it with the fuzz.Funcs(...) generation function | ||
| func GenHistoryEvent(o *types.HistoryEvent, c fuzz.Continue) { | ||
| t1 := int64(1704127933) |
There was a problem hiding this comment.
maybe something more understandable at a glance?
| t1 := int64(1704127933) | |
| t1 := MustParse(DateOnly, "2024-01-01").Unix() |
| @@ -328,6 +328,7 @@ func CopyWorkflowExecutionInfo(sourceInfo *persistence.WorkflowExecutionInfo) *p | |||
| ParentDomainID: sourceInfo.ParentDomainID, | |||
| ParentWorkflowID: sourceInfo.ParentWorkflowID, | |||
| ParentRunID: sourceInfo.ParentRunID, | |||
| IsCron: sourceInfo.IsCron, | |||
There was a problem hiding this comment.
bleh. I really need to try to finish a "this struct must be completely filled out" linter...
There was a problem hiding this comment.
linter or unit test?
one option could be to write tests using cmp.Diff and pass source object fully populated. to make it future proof, test can also validate all the public fields in the source object tree is non-nil.
There was a problem hiding this comment.
That's fundamentally what I'm looking to enable with the generator here - the filling out completely thing.
As Steven mentions, an alternative would be some kind of linter which checks for unfilled / nil substructs or subfields. I worry though that that's a somewhat more intrusive / homebrewed solution requiring some annotation thing we come up with.
Pull Request Test Coverage Report for Build 018e129e-e76b-458d-8d72-096f650c58caDetails
💛 - Coveralls |
davidporter-id-au
force-pushed
the
bugfix/adding-some-missing-coverage-to-copy-utils
branch
from
2833306
to
64311a3
Compare
Codecov Report
Additional details and impacted files
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
…til (cadence-workflow#5572) - Adds the library github.com/google/gofuzz. - Adds a small wrapper for some repo-level conventions we may wish to follow by default - notably determinitism and logging the seed value - Adds some probably fairly low value but still easy to test copy functions as a demonstration of value.
What changed?
github.com/google/gofuzz.Justification & intent
The problem I am trying to solve here is: How can we ensure that testdata is kept up-to-date and in-sync with the changing functions.
A canonical example is idl mapping, where a mapper for Protobuf generated type and it's
common/typesequivalent need to be kept in sync as the fields evolve and additional fields and substructs are added. Added test-data will continue to pass tests, but may silently omit new fields or not handle nil values safely as assumptions about what data must be present correctly.Adding a 'fuzz' or generator for data makes such tests relatively easy: A round-trip test for a serialiser with generated code will easily spot missed values and generated code with occasional nils will ensure safe nil handling before deep-reaching into subfields.
Comparison vs alternatives
This is certainly not the only way to solve this problem: There are several obvious other ways to solve this:
a) straightforward reflection on existing testdata to ensure all fields are present with some kind of linter (rejected: Requires inventing a kind of DSL to specify optional fields in lots of scenarios where structs only sometimes exist, significant maintenance overhead)
b) The native golang security fuzzer available on
testing.F(rejected because more adverserial than the problem I am trying to solve and computationally quite expensive to have it build a corpus of known inputs.c) property based testing such as the stdlib
testing/quick(rejected because the API is quite rigid and it's virtually impossible to use it to create nested substructures without adherence to theGenerate()api and seems to completely not work with protobuf generated types (with their private fields)