fixup json snaps and deser - #692
Conversation
| for (var operation : operations) { | ||
| var inputSymbol = symbolProvider.toSymbol(model.expectShape(operation.getInputShape())); | ||
| for (var operation : allOperations(model, service)) { | ||
| var reason = skipReason(model, operation, eventStreamIndex); |
There was a problem hiding this comment.
praise: I like this reason mechanism
| private static boolean isCbor(GoSettings settings) { | ||
| return Rpcv2CborTrait.ID.equals(settings.getProtocol()); | ||
| // Emits serdeBodyEqual, the request-body comparator, per protocol. Every variant falls back to a byte compare | ||
| // when either side won't decode, so a malformed golden fails loudly rather than silently passing. |
There was a problem hiding this comment.
what's a malformed golden?
There was a problem hiding this comment.
golden = snapshot file
| """); | ||
| } | ||
| // rpcv2Cbor encodes struct fields as a CBOR map and the encoder emits map entries in Go map iteration order, so | ||
| // the same input produces different byte orderings across runs. Comparing decoded values is order-independent. |
There was a problem hiding this comment.
we don't have to resolve it here, but this has also been a point of friction on JSON encoders. Since we are now taking more control of the encoders, would it be worth it to sort map entries?
There was a problem hiding this comment.
by default, a million percent no, it's bad for performance,
as an option, such that we could turn it on for snapshots - 100% and i intend to do so. we have an open issue for this over in the sdk.
There was a problem hiding this comment.
ok, as an option it sounds good.
by default, a million percent no, it's bad for performance
Can't argue with this, but we've had multiple issues with serialization not being deterministic being an issue even if it's not a guarantee for JSON, so it's definitely a tredoff.
Interestingly, JSON v2 switched this behavior and now map keys are not deterministic by default and you have to pass a deterministic flag https://pkg.go.dev/encoding/json/v2#Deterministic
| // A test that exists only to record, in the generated SDK, that an operation or error has no snapshot coverage. | ||
| private Writable writeSkip(String testName, String reason) { | ||
| return goTemplate(""" | ||
| func $name:L(t *$testingT:T) { |
There was a problem hiding this comment.
same, like this placeholder test with operation in signature and reason in log
3 participants
fixup all awsjson/restjson response snapshot issues
serde bugs
nullsnapshot bugs
@streamingblobs in payloads__typein the body instead ofX-Amzn-ErrorType. the header is the highest-precedence source, so it was the only one we ever covered, and the body is what services actually send.codebefore__typeand schema-serde does the opposite, so a placeholder there resolves to a different type under each, to make them consistent, I just made any code field in an error member also carry the error shape nameCompareValueshad noreflect.Interfacecase, so union and document fields fell through toDeepEqualand skipped the reader/document comparators