[common] Add unit test for convertions #1395
Conversation
3vilhamster
requested review from
Groxx,
shijiesheng,
jakobht,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
This comment was marked as resolved.
This comment was marked as resolved.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Continue to review full report in Codecov by Sentry.
|
| // TDeserialize is used to deserialize []byte to thrift TStruct | ||
| func TDeserialize(ctx context.Context, t thrift.TStruct, b []byte) (err error) { | ||
| return thrift.NewTDeserializer().Read(ctx, t, b) | ||
| return t.Transport.Bytes(), t.Protocol.Flush(ctx) |
There was a problem hiding this comment.
this (and not resetting) is a rather significant set of changes - is there something that implies it's correct? e.g. newer thrift/tchannel docs?
There was a problem hiding this comment.
I've looked into thrift.NewTSerializer() and it uses TMemoryBuffer that has a comment of Flush is Noop for it, since it uses plain buffer.
Also, Protocol.Flush is just a wrapper around Transport.Flush().
There was a problem hiding this comment.
// Flushing a memory buffer is a no-op
func (p *TMemoryBuffer) Flush(ctx context.Context) error {
return nil
}
There was a problem hiding this comment.
We always initialize one buffer with
t := thrift.NewTSerializer()
Why it should be reset?
There was a problem hiding this comment.
Why would it be reset: I agree it doesn't look correct, but the only thing that matters is if it is correct. If that hasn't been verified, it needs to be, but hasn't been mentioned anywhere so I'm not sure if it has been. By default we kinda have to assume the current code exists for some reason, unless it's clearly wrong.
There was a problem hiding this comment.
From reading the source code: yea looks fine.
I do think this deserves a comment though, as under normal circumstances this reads like:
bytes := t.Transport.Bytes()
err := t.Transport.Flush(ctx)
return bytes, errwhich means to get the bytes before flushing, which is generally nonsense. It's only valid for the specific in-memory implementation that's being used right now, not for the interface in general.
If we have a way to make sure that's always correct, that'd be worth doing, otherwise tbh I think I prefer the explicit branching :\ it'll catch errors on upgrades that might change this, where right now it'll probably just silently drop data.
| for i := 0; i < len(objs); i++ { | ||
| if !IsThriftType(objs[i]) { | ||
| // NOTE: our criteria to use which encoder is simple if all the types are serializable using thrift then we use | ||
| // thrift encoder. For everything else we default to gob. |
There was a problem hiding this comment.
gob json (we don't use gob anywhere), but this is just a move so it's probably fine. we'll fix it eventually.
What changed?
I've introduced a new helper function PtrOf that could be used instead of helpers. I've left helpers for now since they are used all over the code.
Also, I added tests to conversions and thrift encoders.
Why?
Improving code coverage and logic
How did you test it?
unit tests and code stability