-
Notifications
You must be signed in to change notification settings - Fork 371
Add Spector cases for serializing lossy duration encodings as integers #10835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
f1ccfbe
fa48c47
9aabc3f
fa87ef4
b9a6161
7a11acd
bd267b1
a96701d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,20 @@ namespace Query { | |
| input: duration, | ||
| ): NoContentResponse; | ||
|
|
||
| @route("/int32-seconds-fractional") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
| Test int32 seconds encode for a duration parameter whose value has a fractional (sub-second) component. | ||
| The duration is 35.625 seconds, e.g. TimeSpan.FromSeconds(35.625) in C#. | ||
| Even though the underlying value is fractional, the client must serialize it as an integer (not a floating point number), e.g. `input=36`. | ||
| Expected query parameter to be an integer (the fractional part is dropped during serialization). | ||
| """) | ||
| op int32SecondsFractional( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this (and potentially the milli test) to another category, this doesn't feel like this belongs under
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with doing this is that we still want to have header/query/property variants for the lossy encoding tests to exercise all code paths.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need all 3 I would just keep a single in a body. |
||
| @query | ||
| @encode(DurationKnownEncoding.seconds, int32) | ||
| input: duration, | ||
| ): NoContentResponse; | ||
|
|
||
| @route("/int32-seconds-larger-unit") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
|
|
@@ -202,6 +216,11 @@ namespace Property { | |
| value: duration; | ||
| } | ||
|
|
||
| model Int32SecondsFractionalDurationProperty { | ||
| @encode(DurationKnownEncoding.seconds, int32) | ||
| value: duration; | ||
| } | ||
|
|
||
| model FloatSecondsDurationProperty { | ||
| @encode(DurationKnownEncoding.seconds, float) | ||
| value: duration; | ||
|
|
@@ -320,6 +339,29 @@ namespace Property { | |
| """) | ||
| op int32Seconds(@body body: Int32SecondsDurationProperty): Int32SecondsDurationProperty; | ||
|
|
||
| @route("/int32-seconds-fractional") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
| Test operation with request and response model contains a duration property with int32 seconds encode whose value has a fractional (sub-second) component. | ||
| The duration is 35.625 seconds, e.g. TimeSpan.FromSeconds(35.625) in C#. | ||
| Even though the underlying value is fractional, the client must serialize it as an integer (not a floating point number). | ||
| Expected request body: | ||
| ```json | ||
| { | ||
| "value": 36 | ||
| } | ||
| ``` | ||
| Expected response body: | ||
| ```json | ||
| { | ||
| "value": 36 | ||
| } | ||
| ``` | ||
| """) | ||
| op int32SecondsFractional( | ||
| @body body: Int32SecondsFractionalDurationProperty, | ||
| ): Int32SecondsFractionalDurationProperty; | ||
|
|
||
| @route("/float-seconds") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
|
|
@@ -603,6 +645,20 @@ namespace Header { | |
| duration: duration, | ||
| ): NoContentResponse; | ||
|
|
||
| @route("/int32-seconds-fractional") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
| Test int32 seconds encode for a duration header whose value has a fractional (sub-second) component. | ||
| The duration is 35.625 seconds, e.g. TimeSpan.FromSeconds(35.625) in C#. | ||
| Even though the underlying value is fractional, the client must serialize it as an integer (not a floating point number), e.g. `duration: 36`. | ||
| Expected header to be an integer (the fractional part is dropped during serialization). | ||
| """) | ||
| op int32SecondsFractional( | ||
| @header | ||
| @encode(DurationKnownEncoding.seconds, int32) | ||
| duration: duration, | ||
| ): NoContentResponse; | ||
|
|
||
| @route("/int32-seconds-larger-unit") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from discussion on the issue I don't think we can add a test for this as floor vs round is not something TypeSpec should have an opinion here and it more of an emitter implementation detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test can still validate that the proper number type is serialized (e.g. integer rather than double/decimal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the scenarios to validate only the number type. The mock handlers reject a floating point value and assert an integer is serialized, without checking an exact value. I also reworded the scenarioDocs to state that floor vs. round is an emitter implementation detail and that the test only validates that an integer is sent (e.g.
35or36). Done in 4b13fac.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot actually we can just use input data such that the rounding vs truncating decision wouldn't matter - e.g. 36.25.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by that logic should we have a test to make sure you don't somehow send a string instead of a int as well? It just feels like this is one case you caught here but there is an infinite number of potential failures here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an emitter could also very well here decide that is this an error (crash) so can never get this test to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These type violations are already covered by every round trip test. This case is special because the source type carries more precision than the target encoding. This is a lossy encode scenario - not arbitrary type mismatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contract is "encode this duration as int32 seconds," so crashing is non-conformant behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hhm alright fair enough, though do we need the same for milliseconds encoding?