Fix double negative nan test, ignoring sign#11506
Conversation
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.
|
daily module test: https://github.com/enjoy-binbin/redis/actions/runs/3460242659
|
|
i don't think you can trigger an extra-ci.. it requires a self-hosted runner |
|
yes... i see, it hung |
|
nice, all passed |
oranagra
left a comment
oranagra
left a comment
There was a problem hiding this comment.
i think we need to use * in both the positive and negative.
add a comment that on some platforms one of these can be -nan but we don't care since they are synonym.
i'm having second thoughts about my request for case-insensitive matching request.
I realize that ignoring the case in our tests means our protocol is inconclusive and may need to be changed as well.
considering this (case) wasn't a problem, maybe we should revert it?
on the other hand see this https://en.wikipedia.org/wiki/NaN#Display
maybe the issue with this is just waiting around the corner for us to test on yet another platform.
maybe we should go back on this, and modify redis to normalize all forms of NaNs (including negative) into one when generating the protocol.
@yossigo WDYT?
|
i use * in both the positive and negative and revert the case-insensitive change. (agree with both of your comment)
this feels like a good idea. i will do the commit (* and case one) first, and then let's wait for the discussion about the protocol |
|
i'll merge this PR right away to silence the CI failures, but let's keep the discussion about which types of |
oranagra
changed the title
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.
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.
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.
2 participants
The test introduced in #11482 fail on ARM (extra CI):
It looks like there is no nagative nan on ARM.