Add test to cover NAN reply using a module#11482
Add test to cover NAN reply using a module#11482oranagra merged 2 commits intoredis:unstablefrom oranagra:nan_reply
Conversation
|
For some reason that's not yet clear to me, the reply i get is |
| assert_equal "-nan" [r rw.double 0 0] | ||
| assert_equal "nan" [r rw.double] |
There was a problem hiding this comment.
Negative NaN??? What is that...
There was a problem hiding this comment.
apparently that's what you get from redis (or actually both glibc and the alternative we added in #10587).
so either we "fix" it with a breaking change, or document it.
There was a problem hiding this comment.
Fine then. Apparently the sign bit in a NaN is usually ignored but not always... Wikipedia:
There was a problem hiding this comment.
ohh, now i see the Negative NaN also break ours extra-cli
https://github.com/redis/redis-extra-ci/actions/runs/3457946091/jobs/5771873230#step:8:846
*** [err]: RESP2: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl
Expected '-nan' to be equal to 'nan' (context: type eval line 3 cmd {assert_equal "-nan" [r rw.double 0 0]} proc ::test)
*** [err]: RESP3: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl
Expected ',-nan' to be equal to ',nan' (context: type eval line 8 cmd {assert_equal ",-nan" [r rw.double 0 0]} proc ::test)
There was a problem hiding this comment.
i'm careless.. i should have tested this on Mac and ARM before merging.
There was a problem hiding this comment.
let's do pattern matching to search any kind of nan, ignoring character case and sign.
@enjoy-binbin can you make a PR?
|
PR was approved in a core-team meeting (keeping both positive and negative NaN). |
| assert_equal "-inf" [r rw.double -inf] | ||
| } else { | ||
| assert_equal Inf [r rw.double inf] | ||
| assert_equal -Inf [r rw.double -inf] |
There was a problem hiding this comment.
https://github.com/redis/redis/actions/runs/3457509357/jobs/5770995313#step:6:906
look like this one is not the same on mac
*** [err]: RESP3: RM_ReplyWithDouble: inf in tests/unit/moduleapi/reply.tcl
Expected 'Inf' to be equal to 'inf' (context: type eval line 6 cmd {assert_equal Inf [r rw.double inf]} proc ::test)
ok, i see this will work, i am making the PR: #11504
https://github.com/enjoy-binbin/redis/actions/runs/3458050562/jobs/5772080570#step:6:608
# TCL convert inf to different results on different platforms, e.g. inf on mac
# and Inf on others, so use readraw to verify the protocol
r readraw 1
assert_equal ",inf" [r rw.double inf]
assert_equal ",-inf" [r rw.double -inf]
r readraw 0
The test introduced in redis#11482 fail on mac: ``` *** [err]: RESP3: RM_ReplyWithDouble: inf in tests/unit/moduleapi/reply.tcl Expected 'Inf' to be equal to 'inf' (context: type eval line 6 cmd {assert_equal Inf [r rw.double inf]} proc ::test) ``` Looks like the mac platform returns inf instead of Inf in this case, this PR uses readraw to verify the protocol.
The test introduced in #11482 fail on mac: ``` *** [err]: RESP3: RM_ReplyWithDouble: inf in tests/unit/moduleapi/reply.tcl Expected 'Inf' to be equal to 'inf' (context: type eval line 6 cmd {assert_equal Inf [r rw.double inf]} proc ::test) ``` Looks like the mac platform returns inf instead of Inf in this case, this PR uses readraw to verify the protocol.
The test introduced in redis#11482 fail on ARM (extra CI): ``` *** [err]: RESP2: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl Expected '-nan' to be equal to 'nan' (context: type eval line 3 cmd {assert_equal "-nan" [r rw.double 0 0]} proc ::test) *** [err]: RESP3: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl Expected ',-nan' to be equal to ',nan' (context: type eval line 8 cmd {assert_equal ",-nan" [r rw.double 0 0]} proc ::test) ``` It looks like there is no nagative nan on ARM. In this PR, we add a new `assert_match_nocase` to do the matching, ignoring case and sign.
The test introduced in #11482 fail on ARM (extra CI): ``` *** [err]: RESP2: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl Expected '-nan' to be equal to 'nan' (context: type eval line 3 cmd {assert_equal "-nan" [r rw.double 0 0]} proc ::test) *** [err]: RESP3: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl Expected ',-nan' to be equal to ',nan' (context: type eval line 8 cmd {assert_equal ",-nan" [r rw.double 0 0]} proc ::test) ``` It looks like there is no negative nan on ARM.
|
@yossigo @madolson PTAL personally, i think we better normalize it by calling |
|
according to the comments above, just for test, i made this change: 2614b68 btw, in this case, i guess we need to revert the assert_match part in #11506, use assert_equal just like inf test module test passed in local and triggered a full daily CI to see: https://github.com/enjoy-binbin/redis/actions/runs/3529145166 |
|
@oranagra I thought the point was that we still needed to document that for compatibility other values of nan might be returned. I have no problem with normalizing the changes moving forward. |
|
Normalizing seems better considering we already normalize void addReplyDouble(client *c, double d) {
if (isinf(d)) {
/* Libc in odd systems (Hi Solaris!) will format infinite in a
* different way, so better to handle it in an explicit way. */
if (c->resp == 2) {
addReplyBulkCString(c, d > 0 ? "inf" : "-inf");
} else {
addReplyProto(c, d > 0 ? ",inf\r\n" : ",-inf\r\n",
d > 0 ? 6 : 7);
} |
|
We discussed this again in a core-team meeting. |
From https://en.wikipedia.org/wiki/NaN#Display, it says that apart from nan and -nan, we can also get NAN and even nan(char-sequence) from libc. In redis#11482, our conclusion was that we wanna normalize it in Redis to a single nan type, like we already normalized inf. For this, we also reverted the assert_match part of the test added in redis#11506, using assert_equal to validate the changes.
From https://en.wikipedia.org/wiki/NaN#Display, it says that apart from nan and -nan, we can also get NAN and even nan(char-sequence) from libc. In #11482, our conclusion was that we wanna normalize it in Redis to a single nan type, like we already normalized inf. For this, we also reverted the assert_match part of the test added in #11506, using assert_equal to validate the changes.
|
ok, redis fixed, someone wanna attempt to have a go at the RESP spec or redis-doc (i didn't bother to check what's the scope) |
|
RESP spec: redis/redis-specifications#11 |
Adding a test to cover the already existing behavior of NAN replies, to accompany the PR that adds them to the RESP3 spec: redis/redis-specifications#10 This PR also covers Inf replies that are already in the spec, as well as RESP2 coverage.
The test introduced in redis#11482 fail on mac: ``` *** [err]: RESP3: RM_ReplyWithDouble: inf in tests/unit/moduleapi/reply.tcl Expected 'Inf' to be equal to 'inf' (context: type eval line 6 cmd {assert_equal Inf [r rw.double inf]} proc ::test) ``` Looks like the mac platform returns inf instead of Inf in this case, this PR uses readraw to verify the protocol.
The test introduced in redis#11482 fail on ARM (extra CI): ``` *** [err]: RESP2: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl Expected '-nan' to be equal to 'nan' (context: type eval line 3 cmd {assert_equal "-nan" [r rw.double 0 0]} proc ::test) *** [err]: RESP3: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl Expected ',-nan' to be equal to ',nan' (context: type eval line 8 cmd {assert_equal ",-nan" [r rw.double 0 0]} proc ::test) ``` It looks like there is no negative nan on ARM.
From https://en.wikipedia.org/wiki/NaN#Display, it says that apart from nan and -nan, we can also get NAN and even nan(char-sequence) from libc. In redis#11482, our conclusion was that we wanna normalize it in Redis to a single nan type, like we already normalized inf. For this, we also reverted the assert_match part of the test added in redis#11506, using assert_equal to validate the changes.
Adding a test to cover the already existing behavior of NAN replies, to accompany the PR that adds them to the RESP3 spec: redis/redis-specifications#10 This PR also covers Inf replies that are already in the spec, as well as RESP2 coverage.
The test introduced in redis#11482 fail on mac: ``` *** [err]: RESP3: RM_ReplyWithDouble: inf in tests/unit/moduleapi/reply.tcl Expected 'Inf' to be equal to 'inf' (context: type eval line 6 cmd {assert_equal Inf [r rw.double inf]} proc ::test) ``` Looks like the mac platform returns inf instead of Inf in this case, this PR uses readraw to verify the protocol.
The test introduced in redis#11482 fail on ARM (extra CI): ``` *** [err]: RESP2: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl Expected '-nan' to be equal to 'nan' (context: type eval line 3 cmd {assert_equal "-nan" [r rw.double 0 0]} proc ::test) *** [err]: RESP3: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl Expected ',-nan' to be equal to ',nan' (context: type eval line 8 cmd {assert_equal ",-nan" [r rw.double 0 0]} proc ::test) ``` It looks like there is no negative nan on ARM.
From https://en.wikipedia.org/wiki/NaN#Display, it says that apart from nan and -nan, we can also get NAN and even nan(char-sequence) from libc. In redis#11482, our conclusion was that we wanna normalize it in Redis to a single nan type, like we already normalized inf. For this, we also reverted the assert_match part of the test added in redis#11506, using assert_equal to validate the changes.
5 participants
Adding a test to cover the already existing behavior of NAN replies, to accompany the PR that adds them to the RESP3 spec: redis/redis-specifications#10
This PR also covers Inf replies that are already in the spec, as well as RESP2 coverage.