Fix some RestJson query string protocol request tests for servers #1040
Conversation
From a server perspective, `params` is the expected output after request deserialization. Since `queryParamsMapOfStringList` is bound with `@httpQueryParams` to the query string, its value must be present and derived from the input `queryParams` field of the request test.
| @@ -139,6 +139,10 @@ apply AllQueryStringTypes @httpRequestTests([ | |||
| params: { | |||
| queryFloat: "NaN", | |||
There was a problem hiding this comment.
This test doesn't make sense from the server perspective anyway, since in IEEE 754 floating point numbers, NaN != NaN. As such, what should we assert we received after request deserialization, if we can't compare the deserialized value to anything? Shall I tag this test so that it only applies to the client, or should I assert something special in this case?
There was a problem hiding this comment.
The behavior that nan isn't equal to nan is only important when you're using actual numbers for number things. Here we are asserting that the result is NaN instead of 0, +Inf, -Inf, etc.
There was a problem hiding this comment.
@JordonPhillips I think what you're saying is that a protocol test involving NaN only cares that the deserialized value is a NaN and that we shouldn't compare the bits, right? That's the approach we took https://docs.rs/aws-smithy-protocol-test/latest/aws_smithy_protocol_test/trait.FloatEquals.html, let me know if something else should be asserted.
In any case, this is subtle and should be called out in the documentation, perhaps in this page? https://awslabs.github.io/smithy/1.0/spec/http-protocol-compliance-tests.html
| @@ -139,6 +139,10 @@ apply AllQueryStringTypes @httpRequestTests([ | |||
| params: { | |||
| queryFloat: "NaN", | |||
There was a problem hiding this comment.
The behavior that nan isn't equal to nan is only important when you're using actual numbers for number things. Here we are asserting that the result is NaN instead of 0, +Inf, -Inf, etc.
The fixes were upstreamed in smithy-lang/smithy#1040, which was rolled out long ago in Smithy v1.18, 10 months ago.
The fixes were upstreamed in smithy-lang/smithy#1040, which was rolled out long ago in Smithy v1.18, 10 months ago.
Fixes were upstreamed in: * smithy-lang/smithy#1040, which was rolled out in Smithy v1.18, 10 months ago. * smithy-lang/smithy#1392, which was rolled out in Smithy v1.26.1, 9 weeks ago.
Fixes were upstreamed in: * smithy-lang/smithy#1040, which was rolled out in Smithy v1.18, 10 months ago. * smithy-lang/smithy#1392, which was rolled out in Smithy v1.26.1, 9 weeks ago.
Fixes were upstreamed in: * smithy-lang/smithy#1040, which was rolled out in Smithy v1.18, 10 months ago. * smithy-lang/smithy#1392, which was rolled out in Smithy v1.26.1, 9 weeks ago.
From a server perspective,
paramsis the expected output after requestdeserialization. Since
queryParamsMapOfStringListis bound with@httpQueryParamsto the query string, its value must be present andderived from the input
queryParamsfield of the request test.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.