Skip to content
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

Fix test failure due to differing reply format of XREADGROUP under RESP3 in MULTI #13255

Merged
merged 4 commits into from May 14, 2024

Conversation

sundb
Copy link
Collaborator

@sundb sundb commented May 10, 2024

This test was introducted by #13251.
Normally we auto transform the reply format of XREADGROUP to array under RESP3 (see trasformer_funcs).
But when we execute XREADGROUP command in multi it can't work, which cause the new test failed.
The solution is to verity the reply of XREADGROUP in advance rather than in MULTI.

Failed validate schema CI: https://github.com/redis/redis/actions/runs/9025128323/job/24800285684

@sundb sundb requested a review from guybe7 May 10, 2024 07:52
@sundb
Copy link
Collaborator Author

sundb commented May 10, 2024

Fully CI only with flag --single unit/type/stream-cgroups passed: https://github.com/sundb/redis/actions/runs/9029242361

@guybe7
Copy link
Collaborator

guybe7 commented May 10, 2024

@sundb aren't we running all tests with RESP3 by default when we verify the reply schema?

$master del mystream
$master xadd mystream 1-0 a b c d e f
$master xgroup create mystream mygroup 0
$master xreadgroup group mygroup ryan count 1 streams mystream >
$master multi
$master xreadgroup group mygroup ryan count 1 streams mystream 0
set reply [$master exec]
assert_equal $reply {{{mystream {{1-0 {a b c d e f}}}}}}
assert_equal $reply {{mystream {{1-0 {a b c d e f}}}}}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guybe7 yes, but the test failed under RESP3, so I changed the test to always run under RESP3.
please note that i removed a {}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason why i don't fix it under RESP2 is hello 2 doesn't work with flag --force-resp3.

@sundb
Copy link
Collaborator Author

sundb commented May 11, 2024

@guybe7 I converted the map to array to consistency with RESP2.

@guybe7
Copy link
Collaborator

guybe7 commented May 14, 2024

@sundb i don't understand why is XREADGROUP inside MULTI different from any other command inside MULTI

@sundb
Copy link
Collaborator Author

sundb commented May 14, 2024

@guybe7 It's not different, we had never verify the command that reply a map inside the multi.
Not many commands reply a map.

@guybe7
Copy link
Collaborator

guybe7 commented May 14, 2024

@sundb so we have a general problem with validating the reply schema of map-reply inside an EXEC reply?

@sundb
Copy link
Collaborator Author

sundb commented May 14, 2024

@guybe7 yes, another alternative is to add an automatic verversion to exec command to convert all map replies into array,
i'm not sure it's worth.

@guybe7
Copy link
Collaborator

guybe7 commented May 14, 2024

ok so the problem is that transform_response_if_needed won't work because argv[0] is EXEC and not any of the commands whose responses should be translated from RESP3 to RESP2 (see trasformer_funcs)

it's a deep issue, i think it's better to quick-fix it like this:

diff --git a/tests/unit/type/stream-cgroups.tcl b/tests/unit/type/stream-cgroups.tcl
index 2462a25ba..d5754d42b 100644
--- a/tests/unit/type/stream-cgroups.tcl
+++ b/tests/unit/type/stream-cgroups.tcl
@@ -1398,11 +1398,10 @@ start_server {
             $master del mystream
             $master xadd mystream 1-0 a b c d e f
             $master xgroup create mystream mygroup 0
-            $master xreadgroup group mygroup ryan count 1 streams mystream >
+            assert_equal [$master xreadgroup group mygroup ryan count 1 streams mystream >] {{mystream {{1-0 {a b c d e f}}}}}
             $master multi
             $master xreadgroup group mygroup ryan count 1 streams mystream 0
-            set reply [$master exec]
-            assert_equal $reply {{{mystream {{1-0 {a b c d e f}}}}}}
+            $master exec
         }
     }

sundb and others added 2 commits May 14, 2024 19:40
Co-authored-by: guybe7 <guy.benoish@redislabs.com>
@sundb
Copy link
Collaborator Author

sundb commented May 14, 2024

@guybe7 thanks, apply your patch.

@sundb sundb merged commit ffbdf2f into redis:unstable May 14, 2024
13 checks passed
@sundb sundb deleted the fix_xreadgroup_reply_test branch May 14, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants