Add Spector cases for serializing lossy duration encodings as integers#10835
Add Spector cases for serializing lossy duration encodings as integers#10835Copilot wants to merge 8 commits into
Conversation
Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
There was a problem hiding this comment.
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.
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.
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. 35 or 36). Done in 4b13fac.
There was a problem hiding this comment.
@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.
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.
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.
by that logic should we have a test to make sure you don't somehow send a string instead of a int as well?
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.
an emitter could also very well here decide that is this an error (crash) so can never get this test to work
The contract is "encode this duration as int32 seconds," so crashing is non-conformant behavior.
There was a problem hiding this comment.
hhm alright fair enough, though do we need the same for milliseconds encoding?
…ding Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
…t value Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
commit: |
|
@copilot run chronus add |
Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
|
You can try these changes here
|
|
@copilot add a line to the test description clarifying that this is specifically exercising the lossy encode scenario and not about arbitrary type mismatches. |
Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
Added a line to each of the three scenario docs (Query, Property, Header) clarifying that this specifically exercises the lossy encode case where the source type carries more precision than the target encoding, and is not about arbitrary type mismatches. Done in the latest commit. |
There was a problem hiding this comment.
hhm alright fair enough, though do we need the same for milliseconds encoding?
| This scenario specifically exercises the lossy encode case where the source type carries more precision than the target encoding; it is not about arbitrary type mismatches, which are already covered by other round-trip scenarios. | ||
| The value is chosen so that rounding and truncating both yield the same integer, so the expected query parameter is `input=36`. | ||
| """) | ||
| op int32SecondsFractional( |
There was a problem hiding this comment.
Can we move this (and potentially the milli test) to another category, this doesn't feel like this belongs under query encoding. Maybe we can have a loosy encoding test file dedicated to those kind of tests)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't think we need all 3 I would just keep a single in a body.
| value, | ||
| ); | ||
| } | ||
| if (value !== 36) { |
There was a problem hiding this comment.
probably unlikely but an implementation could also choose ceil so we might want to allow 37 to be explicit that all of this is allowed.
An option for those kind of tests would also be to have a 2-phase test, first part you get a value(randomly generated for that server instance) maybe duration encoded as float then you got to send back that value.
|
@copilot please apply the feedback |
…liseconds, allow ceil Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
Applied the feedback:
Regenerated |
|
Error: test/e2e/http/encode/duration/main.test.ts(13,25): error TS2339: Property 'default_' does not exist on type 'QueryClient'. |
…y ops @post Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
Fixed. The failure was a client-name collision: the |
| The client must serialize the value as an integer (not a floating point number such as `36.25`), discarding the sub-second precision. | ||
| Because emitters may floor, round, or ceil when discarding that precision, the expected query parameter is `input=36` or `input=37`. | ||
| """) | ||
| op queryInt32Seconds( |
There was a problem hiding this comment.
I am not sure we really need for header, property, query, etc.
I think we should just need 2 tests
interface Lossy {
op intSeconds(
@query
@encode(DurationKnownEncoding.seconds, int32)
input: duration
): void;
op intMilliseconds(
@query
@encode(DurationKnownEncoding.milliseconds, int32)
input: duration
): void;
}with the http route matching
Existing
encode/durationscenarios only used whole-number payloads, so they couldn't catch the bug from #10831 where adurationwith a fractional sub-second component was serialized as a floating point number (e.g.123.45) instead of an integer.These tests target the lossy encode case, where the source
durationcarries more precision than the target integer encoding can represent. The client must still serialize the value using the target number type (an integer), discarding the extra precision, rather than emitting a floating point number. This is distinct from arbitrary type mismatches, which are already covered by the existing round-trip scenarios.Changes (
packages/http-specs/specs/encode/duration/)main.tsp: New dedicatedLossynamespace (routed under/encode/duration/lossy/...) containing the lossy-encode scenarios, instead of placing them under the regularQuery/Property/Headerencoding categories. For each transport (query, property, header) it adds:int32-seconds: a36.25-second duration encoded asint32seconds (sub-second source precision).int32-milliseconds: a36250.25-millisecond duration encoded asint32milliseconds (sub-millisecond source precision).mockapi.ts: New handlers (createLossyBodyServerTests,createLossyQueryServerTests,createLossyHeaderServerTests) verify that the received value is serialized as an integer (rejecting float/double/decimal output). They accept an allow-list of integers covering floor, round, and ceil results (36/37for seconds,36250/36251for milliseconds), so the test verifies the core contract ("serializes as an int") without taking a position on an emitter's rounding mode.spec-summary.md: Regenerated.Note
The mock validation accepts floor/round/ceil results rather than a single exact value, so it remains agnostic to an emitter's rounding mode while still failing on floating point output.