Skip to content

Improve #ec3 Blocking retrieval #219

Merged
andy1li merged 5 commits intomainfrom
andy/tweak
Aug 28, 2025
Merged

Improve #ec3 Blocking retrieval #219
andy1li merged 5 commits intomainfrom
andy/tweak

Conversation

@andy1li
Copy link
Member

@andy1li andy1li commented Aug 27, 2025

  1. Send commands in ascending order of client number

  2. Assert the NoExpectedResponse before the BLPOP response

image

@andy1li andy1li self-assigned this Aug 27, 2025
@andy1li andy1li requested a review from rohitpaulk August 27, 2025 02:43
…esponse assertion method. Introduce AssertResponsesInReverseOrder for handling response assertions in reverse. Update logging for no response expectations in various test fixtures.
@andy1li andy1li added the regenerate-fixtures Trigger a CI job to regenerate fixtures label Aug 27, 2025
@github-actions
Copy link

Triggered a Github Actions job to update fixtures.

@github-actions github-actions bot removed the regenerate-fixtures Trigger a CI job to regenerate fixtures label Aug 27, 2025
@github-actions
Copy link

Triggered a Github Actions job to update fixtures.

Copy link
Member

@rohitpaulk rohitpaulk left a comment

Choose a reason for hiding this comment

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

Added a q

…n reverse order. Update testListBlpopNoTimeout to utilize the new functionality and adjust response assertion method accordingly. Modify test fixtures to reflect changes in client connection ports.
@andy1li andy1li requested a review from rohitpaulk August 27, 2025 20:38
type BlockingClientGroupTestCase struct {
clientsWithExpectedResponses []clientWithExpectedResponse
clientsWithExpectedResponses []clientWithExpectedResponse
ShouldAssertResponsesInReverseOrder bool
Copy link
Member

Choose a reason for hiding this comment

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

@andy1li my question was more related to do we need to ever control the order of assertions? Can we always use the reverse order? (Does the logic not apply to all stages?)

Copy link
Member Author

Choose a reason for hiding this comment

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

@rohitpaulk Just checked:

  1. When alway asserting in reverse, make test raises no error.

  2. For other stages using BlockingClientGroupTestCase, all include only one expectation, so the order does not make a difference.

So let's go with alway asserting in reverse?

Copy link
Member

Choose a reason for hiding this comment

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

Yep! Can remove the configuration option and keep things simple

Copy link
Member

@rohitpaulk rohitpaulk left a comment

Choose a reason for hiding this comment

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

Added q

…n flag. Update testListBlpopNoTimeout to reflect this change. Adjust test fixtures to ensure consistent client connection ports.
@andy1li andy1li added the regenerate-fixtures Trigger a CI job to regenerate fixtures label Aug 27, 2025
@github-actions
Copy link

Triggered a Github Actions job to update fixtures.

@github-actions github-actions bot removed the regenerate-fixtures Trigger a CI job to regenerate fixtures label Aug 27, 2025
@andy1li andy1li merged commit bb1cac0 into main Aug 28, 2025
3 checks passed
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.

3 participants