Coverage for dataStoreInterfaces #5743
Conversation
This is... much more questionable code than I anticipated going in.
Groxx
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov Report
Additional details and impacted files
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| // I'm not entirely sure why this behavior exists, but to nail it down: | ||
| assert.NotPanics(t, func() { | ||
| NewDataBlob([]byte(problematic), common.EncodingTypeThriftRW) | ||
| }, "only thriftrw data can start with Y without panicking") |
There was a problem hiding this comment.
no reason or comment about this in the original PR: #1176
There was a problem hiding this comment.
Yeah, sorry, no idea either
| // highly suspicious | ||
| unknown(common.EncodingTypeProto) |
There was a problem hiding this comment.
very strange.
also this entire func seems ridiculous to me. it converts EncodingType -> string -> EncodingType and loses data in the process. why in the world doesn't it just return the EncodingType
| assert.EqualValues(t, internal, blob.ToInternal(), "thriftrw should encode to internal type") | ||
| assert.Equal(t, blob, NewDataBlobFromInternal(internal), "thriftrw should decode from internal type") | ||
|
|
||
| // all these panic |
There was a problem hiding this comment.
we don't have enums for this in thrift/proto... beyond that I'm not sure why this is like this. not sure where it's serialized / where it would matter. (I have not really looked though)
| panic(fmt.Sprintf("Invalid incoding: \"%v\"", encodingType)) | ||
| if encodingType != common.EncodingTypeThriftRW && data[0] == 'Y' { | ||
| // original reason for this is not written down, but maybe for handling data prior to an encoding type? | ||
| panic(fmt.Sprintf("Invalid data blob encoding: \"%v\"", encodingType)) |
There was a problem hiding this comment.
all these panics should be replaced by err returns ideally but the blast radius of changes might be too big
There was a problem hiding this comment.
There seems no reason. Temporal removed that logic: https://github.com/temporalio/temporal/pull/638/files#diff-f4778e6c9f74affb5767ad2ce6672a85ac6ba16eb64b42057e7dcef273ed5ac0L641
There was a problem hiding this comment.
we should probably remove it at some point then too.
I'll leave that for another diff / let's make a task or something around it, and see if thrift/proto ever start with Y because what even
Pull Request Test Coverage Report for Build 018e3e63-d019-4d4a-8f62-e1cf499a7e72Details
💛 - Coveralls |
This is... much more questionable code than I anticipated going in.
Not gonna change it in this commit though.