Skip to content

Improve #ec3 Blocking retrieval #219

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.

Sign up for GitHub

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

Merged
andy1li merged 5 commits into from
Aug 28, 2025
Merged

Improve #ec3 Blocking retrieval #219

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f00698e
Refactor testListBlpopNoTimeout to use different clients and update r…
andy1li Aug 27, 2025
b2f51c0
ci: add regenerated fixtures
codecrafters-bot Aug 27, 2025
9aa397a
Refactor BlockingClientGroupTestCase to support response assertions i…
andy1li Aug 27, 2025
e68d253
Refactor BlockingClientGroupTestCase to remove reverse order assertio…
andy1li Aug 27, 2025
4703eec
ci: add regenerated fixtures
codecrafters-bot Aug 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions internal/test_cases/blocking_client_group_test_case.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ type clientWithExpectedResponse struct {
}

type BlockingClientGroupTestCase struct {
clientsWithExpectedResponses []clientWithExpectedResponse
clientsWithExpectedResponses []clientWithExpectedResponse
ShouldAssertResponsesInReverseOrder bool
Copy link
Member

@rohitpaulk rohitpaulk Aug 27, 2025

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

@andy1li andy1li Aug 27, 2025

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

@rohitpaulk rohitpaulk Aug 27, 2025

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

}

func (t *BlockingClientGroupTestCase) AddClientWithExpectedResponse(client *instrumented_resp_connection.InstrumentedRespConnection, command string, args []string, assertion resp_assertions.RESPAssertion) *BlockingClientGroupTestCase {
Expand Down Expand Up @@ -55,7 +56,13 @@ func (t *BlockingClientGroupTestCase) SendBlockingCommands() error {
}

func (t *BlockingClientGroupTestCase) AssertResponses(logger *logger.Logger) error {
for _, clientWithExpectedResponse := range t.clientsWithExpectedResponses {
start, end, step := 0, len(t.clientsWithExpectedResponses), 1
if t.ShouldAssertResponsesInReverseOrder {
start, end, step = len(t.clientsWithExpectedResponses)-1, -1, -1
}

for i := start; (t.ShouldAssertResponsesInReverseOrder && i > end) || (!t.ShouldAssertResponsesInReverseOrder && i < end); i += step {
clientWithExpectedResponse := t.clientsWithExpectedResponses[i]
clientLogger := clientWithExpectedResponse.Client.GetLogger()
if clientWithExpectedResponse.Assertion == nil {
testCase := NoResponseTestCase{}
Expand Down
288 changes: 144 additions & 144 deletions internal/test_helpers/fixtures/geospatial/pass
View file
Open in desktop

Large diffs are not rendered by default.

26 changes: 12 additions & 14 deletions internal/test_helpers/fixtures/lists/blpop_wrong_client
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,20 @@ Debug = true
[tester::#EC3] $ ./spawn_redis_server.sh
[your_program] Logs from your program will appear here!
[your_program] Redis server listening on 0.0.0.0:6379
[tester::#EC3] [client-1] Connected (port 63777 -> port 6379)
[tester::#EC3] [client-2] Connected (port 63778 -> port 6379)
[tester::#EC3] [client-3] Connected (port 63779 -> port 6379)
[tester::#EC3] [client-1] Connected (port 59889 -> port 6379)
[tester::#EC3] [client-2] Connected (port 59890 -> port 6379)
[tester::#EC3] [client-3] Connected (port 59891 -> port 6379)
[tester::#EC3] [client-1] $ redis-cli BLPOP pear 0
[tester::#EC3] [client-1] Sent bytes: "*3\r\n$5\r\nBLPOP\r\n$4\r\npear\r\n$1\r\n0\r\n"
[tester::#EC3] [client-2] $ redis-cli BLPOP pear 0
[tester::#EC3] [client-2] Sent bytes: "*3\r\n$5\r\nBLPOP\r\n$4\r\npear\r\n$1\r\n0\r\n"
[tester::#EC3] [client-3] $ redis-cli BLPOP pear 0
[tester::#EC3] [client-3] Sent bytes: "*3\r\n$5\r\nBLPOP\r\n$4\r\npear\r\n$1\r\n0\r\n"
[tester::#EC3] [client-1] $ redis-cli RPUSH pear orange
[tester::#EC3] [client-1] Sent bytes: "*3\r\n$5\r\nRPUSH\r\n$4\r\npear\r\n$6\r\norange\r\n"
[tester::#EC3] [client-1] Received bytes: ":1\r\n"
[tester::#EC3] [client-1] Received RESP integer: 1
[tester::#EC3] [client-1] ✔︎ Received 1
[tester::#EC3] [client-2] Expecting response of BLPOP command
[tester::#EC3] Received: "" (no content received)
[tester::#EC3]  ^ error
[tester::#EC3] Error: Expected start of a new RESP2 value (either +, -, :, $ or *)
[tester::#EC3] [client-3] $ redis-cli RPUSH pear orange
[tester::#EC3] [client-3] Sent bytes: "*3\r\n$5\r\nRPUSH\r\n$4\r\npear\r\n$6\r\norange\r\n"
[tester::#EC3] [client-3] Received bytes: ":1\r\n"
[tester::#EC3] [client-3] Received RESP integer: 1
[tester::#EC3] [client-3] ✔︎ Received 1
[tester::#EC3] [client-2] Expecting no response
[tester::#EC3] client-2 received unexpected response: "*2\r\n$4\r\npear\r\n$6\r\norange\r\n"
[tester::#EC3] Test failed
[tester::#EC3] Terminating program
[tester::#EC3] Program terminated successfully
Loading