[TestSuit] Fix CanDoSRANDMEMBERWithCountCommandLC test #1143
Conversation
prvyk
force-pushed
the
CanDoSRANDMEMBERTest
branch
from
98ad5a4 to
2a3e5d7
Compare
prvyk
force-pushed
the
CanDoSRANDMEMBERTest
branch
from
2a3e5d7 to
c7bd663
Compare
prvyk
force-pushed
the
CanDoSRANDMEMBERTest
branch
from
c7bd663 to
3f57609
Compare
prvyk
force-pushed
the
CanDoSRANDMEMBERTest
branch
3 times, most recently
from
5eec15f to
0eab7d4
Compare
prvyk
force-pushed
the
CanDoSRANDMEMBERTest
branch
from
0eab7d4 to
790d92c
Compare
test/Garnet.test/RespSetTest.cs
Outdated
|
|
||
| string[] results; | ||
|
|
||
| fixed (byte* p = &response[0]) |
There was a problem hiding this comment.
These changes might be a bit hard to read and also make the innerworkings of LightClient not well hidden.
We're also not validating that the members returned are in the set.
Since we don't have to worry about perf here, I'd say we should just read the members one by one and validate their set membership.
There was a problem hiding this comment.
I would rather avoid yet another copy of RESP parsing code.
Rewrote the test to use GarnetClientSession and it can parse the array for us. Also, added testing that all members returned are in the set.
There was a problem hiding this comment.
Well the original test was testing the command with LightClient, so using GarnetClient (while valuable) is inconsistent with other tests in this context. I understand your concern about duplicated parsing logic, but it's how it's been done throughout the test code, so I think that for consistency's sake we should stick to it until we decide to do some wide overhaul of the LC-related code.
prvyk
force-pushed
the
CanDoSRANDMEMBERTest
branch
from
790d92c to
e23416d
Compare
|
Where are we on the pending requests for this PR? It's been open quite a while, would be good to close on it, thanks! |
|
I need to return the test to lightClient (reverting the last commit or so) and then copy some RespReadUtils code to Garnet.test and adjust to read out of a normal string (we don't care about being ultra fast there). It's not a problem, I was just prioritizing other PRs... |
Please re-request a review when this is ready so I can take a look :) Thanks! |
Fix remaining Encoding.ASCII.GetString usages.
prvyk
force-pushed
the
CanDoSRANDMEMBERTest
branch
from
e23416d to
bfe30bc
Compare
prvyk
force-pushed
the
CanDoSRANDMEMBERTest
branch
from
bfe30bc to
fabe79d
Compare
The test didn't really test for duplicate members, because it didn't update arrLenEndIdx, so the next loop run just compared against the previous - passing regardless of actual behaviour (which happened to be per spec, but the test should still work).
Also fix remaining Encoding.ASCII.GetString usages.