JSON value s11n unit tests #603
Conversation
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add tests for Nan and Inf
There was a problem hiding this comment.
We use the JSON definition of number, Nan and Inf are not numbers.
There was a problem hiding this comment.
But you can set value of NumberElement to Nan/Inf, isn't it?
There should be, at least, defined bahavior for this case
There was a problem hiding this comment.
You can theoretically construct a NumberElement with any string now, as the check for JSON Number is in Serialize.cc. But it will have undefined behaviour, so I'm not sure what to test against. Finding something non-numberish in a NumberElement could be handled as an assert in JsonValue.cc.
There was a problem hiding this comment.
On other side refract is not strictly for JSON, so we should define what happens. e.g. conversion to zero?
Not sure, just idea
There was a problem hiding this comment.
There is the relevant issue on the MSON spec not being spec enough: apiaryio/mson#93
In API Elements ("refract") we are pretty clear: https://github.com/apiaryio/api-elements/blob/master/docs/element-definitions.md#values
We could add a debug build run time check during JSON serialization (not JSON value serialization), although I don't deem that necessary. Here are the tests currently covering your worries: https://github.com/apiaryio/drafter/blob/master/test/test-Serialize.cc#L931
There was a problem hiding this comment.
Ok. Let it pass for now and create issue to resolve this
|
👍 great coverage of testcases |
Includes minor fixes; not all corner cases are covered by integration tests.