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.

@TalZaccai TalZaccai requested review from TalZaccai and Copilot March 11, 2025 18:23

Choose a reason for hiding this comment

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

Pull Request Overview

This PR standardizes the assertion logic for comparing light client responses in tests by introducing a new helper method in TestUtils and replacing manual string slicing and ClassicAssert calls across multiple test files. Key changes include adding the AssertEqualUpToExpectedLength method with aggressive inlining, and refactoring numerous tests to call this method instead of duplicating substring logic.

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

File Description
test/Garnet.test/TestUtils.cs Adds the new assertion method to standardize light client response comparisons.
Multiple test files (e.g. TransactionTests.cs, RespAdminCommandsTests.cs, …) Replaces manual response substring extraction and ClassicAssert equality checks with calls to TestUtils.AssertEqualUpToExpectedLength.
Comments suppressed due to low confidence (2)

test/Garnet.test/TestUtils.cs:96

  • Consider adding a defensive check inside AssertEqualUpToExpectedLength to ensure that the response array contains at least expectedResponse.Length bytes before performing the substring conversion. This can help prevent potential IndexOutOfRange exceptions if the response is unexpectedly short.
[MethodImpl(MethodImplOptions.AggressiveInlining)]

test/Garnet.test/TestUtils.cs:97

  • [nitpick] Consider renaming the method to something like 'AssertResponseStartsWith' to more clearly describe that it compares the beginning of the response with the expected string.
internal static void AssertEqualUpToExpectedLength(string expectedResponse, byte[] response)
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