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

[TestSuit] Use consistent assertion for lightClient responses. #1076

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

prvyk
Copy link
Contributor

@prvyk prvyk commented Mar 7, 2025

LightClient is used for a lot of the testsuit. It uses a large fixed array, which is trimmed to compare a fixed expected responsed. There are at least 4 separate ways this is done in the testsuit**. Also the comparison is sometimes wrong in the argument order (doesn't use expected response as expected) or very rarely just wrong (CanUseZDiffSTORE test).

This PR introduces a standard TestUtils methods for this assertion, changes almost all relevant locations to use it, and fixes the few cases when it's done incorrectly.

**

  1. There's this:
    Encoding.ASCII.GetString(response).Substring(0, expectedResponse.Length).

(Runs GetString + allocation over the entire fixed array).

  1. There's also:
    Encoding.ASCII.GetString(response, 0, expectedResponse.Length)

(Reasonable, but allocates. Maybe could be improved)

  1. And also:
    ClassicAssert.AreEqual(response.AsSpan().Slice(0, expectedResponse.Length).ToArray(), expectedResponse);

(Order of arguments is wrong. Otherwise, it's interesting to note NUnit can handle the different types, not clear if that is better or worse).

  1. Lastly there are calls to lightclient's Execute() method which just does the second under the hood. These aren't modified for now.

This diff uses the Encoding.ASCII.GetString(response, 0, expectedResponse.Length) method. It would be easy the try the latter method (in the correct argument order) if it's useful.

(Re #1063 comments: I don't view this as 'improving lightClient' but as improving the testsuit. If we are stuck with some use for now, might as well test the assertion in a consistent and safe way.)

@prvyk prvyk force-pushed the lightclientresponseassert branch from 0e4f155 to c831a20 Compare March 7, 2025 23:32
@badrishc
Copy link
Collaborator

badrishc commented Mar 8, 2025

(Re #1063 comments: I don't view this as 'improving lightClient' but as improving the testsuit. If we are stuck with some use for now, might as well test the assertion in a consistent and safe way.)

Makes sense. It is worth examining the usage sites of LightClient to make sure it was specifically necessary to use it, versus GCS (GarnetClientSession) or SE.Redis. For example. some tests care about specific network splits which GCS and SE.Redis cannot directly control.

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.

2 participants