Merged
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Six AOF stages have no assertions
- Changed the six empty AOF test functions to return explicit errors indicating the tests are not yet implemented, preventing false passes.
- ✅ Fixed: Dir config assertion lost slash tolerance
- Restored ConfigGetDirResponseAssertion which accepts both the expected path and the path with a trailing slash, and updated all dir config tests to use it.
Or push these changes by commenting:
@cursor push 9f698acce2
Preview (9f698acce2)
diff --git a/internal/resp_assertions/config_get_dir_response_assertion.go b/internal/resp_assertions/config_get_dir_response_assertion.go
new file mode 100644
--- /dev/null
+++ b/internal/resp_assertions/config_get_dir_response_assertion.go
@@ -1,0 +1,46 @@
+package resp_assertions
+
+import (
+ "fmt"
+
+ resp_value "github.com/codecrafters-io/redis-tester/internal/resp/value"
+)
+
+type ConfigGetDirResponseAssertion struct {
+ ExpectedDirValue string
+}
+
+func NewConfigGetDirResponseAssertion(expectedDirValue string) RESPAssertion {
+ return ConfigGetDirResponseAssertion{ExpectedDirValue: expectedDirValue}
+}
+
+func (a ConfigGetDirResponseAssertion) Run(value resp_value.Value) error {
+ arrayTypeAssertion := DataTypeAssertion{ExpectedType: resp_value.ARRAY}
+
+ if err := arrayTypeAssertion.Run(value); err != nil {
+ return err
+ }
+
+ if len(value.Array()) != 2 {
+ return fmt.Errorf("Expected 2 elements in array, got %d (%s)", len(value.Array()), value.FormattedString())
+ }
+
+ firstElement := value.Array()[0]
+ secondElement := value.Array()[1]
+
+ if firstElement.Type != resp_value.BULK_STRING {
+ return fmt.Errorf("Expected element #1 to be a bulk string, got %s", firstElement.Type)
+ }
+ if firstElement.String() != "dir" {
+ return fmt.Errorf("Expected element #1 to be %q, got %q", "dir", firstElement.String())
+ }
+
+ if secondElement.Type != resp_value.BULK_STRING {
+ return fmt.Errorf("Expected element #2 to be a bulk string, got %s", secondElement.Type)
+ }
+ if secondElement.String() != a.ExpectedDirValue && secondElement.String() != a.ExpectedDirValue+"/" {
+ return fmt.Errorf("Expected element #2 to be %q, got %q", a.ExpectedDirValue, secondElement.String())
+ }
+
+ return nil
+}
diff --git a/internal/test_aof_config_defaults.go b/internal/test_aof_config_defaults.go
--- a/internal/test_aof_config_defaults.go
+++ b/internal/test_aof_config_defaults.go
@@ -33,11 +33,11 @@
// Test the default configs used for AOF
multiCommandTestCase := test_cases.MultiCommandTestCase{
CommandWithAssertions: []test_cases.CommandWithAssertion{
+ {
+ Command: []string{"CONFIG", "GET", "dir"},
+ Assertion: resp_assertions.NewConfigGetDirResponseAssertion(currentWorkingDirectory),
+ },
{
- Command: []string{"CONFIG", "GET", "dir"},
- Assertion: resp_assertions.NewConfigGetBulkStringValueAssertion("dir", currentWorkingDirectory),
- },
- {
Command: []string{"CONFIG", "GET", "appendonly"},
Assertion: resp_assertions.NewConfigGetBulkStringValueAssertion("appendonly", "no"),
},
diff --git a/internal/test_aof_config_from_flags.go b/internal/test_aof_config_from_flags.go
--- a/internal/test_aof_config_from_flags.go
+++ b/internal/test_aof_config_from_flags.go
@@ -47,11 +47,11 @@
multiCommandTestCase := test_cases.MultiCommandTestCase{
CommandWithAssertions: []test_cases.CommandWithAssertion{
+ {
+ Command: []string{"CONFIG", "GET", "dir"},
+ Assertion: resp_assertions.NewConfigGetDirResponseAssertion(workingDirectory),
+ },
{
- Command: []string{"CONFIG", "GET", "dir"},
- Assertion: resp_assertions.NewConfigGetBulkStringValueAssertion("dir", workingDirectory),
- },
- {
Command: []string{"CONFIG", "GET", "appendonly"},
Assertion: resp_assertions.NewConfigGetBulkStringValueAssertion("appendonly", "yes"),
},
diff --git a/internal/test_aof_create_aof_directory.go b/internal/test_aof_create_aof_directory.go
--- a/internal/test_aof_create_aof_directory.go
+++ b/internal/test_aof_create_aof_directory.go
@@ -1,7 +1,11 @@
package internal
-import "github.com/codecrafters-io/tester-utils/test_case_harness"
+import (
+ "errors"
+ "github.com/codecrafters-io/tester-utils/test_case_harness"
+)
+
func testAofCreateAofDirectory(_ *test_case_harness.TestCaseHarness) error {
- return nil
+ return errors.New("test not implemented: AOF directory creation assertions are pending")
}
diff --git a/internal/test_aof_create_append_only_file.go b/internal/test_aof_create_append_only_file.go
--- a/internal/test_aof_create_append_only_file.go
+++ b/internal/test_aof_create_append_only_file.go
@@ -1,7 +1,11 @@
package internal
-import "github.com/codecrafters-io/tester-utils/test_case_harness"
+import (
+ "errors"
+ "github.com/codecrafters-io/tester-utils/test_case_harness"
+)
+
func testAofCreateAppendOnlyFile(_ *test_case_harness.TestCaseHarness) error {
- return nil
+ return errors.New("test not implemented: AOF append-only file creation assertions are pending")
}
diff --git a/internal/test_aof_create_manifest_file.go b/internal/test_aof_create_manifest_file.go
--- a/internal/test_aof_create_manifest_file.go
+++ b/internal/test_aof_create_manifest_file.go
@@ -1,7 +1,11 @@
package internal
-import "github.com/codecrafters-io/tester-utils/test_case_harness"
+import (
+ "errors"
+ "github.com/codecrafters-io/tester-utils/test_case_harness"
+)
+
func testAofCreateManifestFile(_ *test_case_harness.TestCaseHarness) error {
- return nil
+ return errors.New("test not implemented: AOF manifest file creation assertions are pending")
}
diff --git a/internal/test_aof_filter_commands_before_write.go b/internal/test_aof_filter_commands_before_write.go
--- a/internal/test_aof_filter_commands_before_write.go
+++ b/internal/test_aof_filter_commands_before_write.go
@@ -1,7 +1,11 @@
package internal
-import "github.com/codecrafters-io/tester-utils/test_case_harness"
+import (
+ "errors"
+ "github.com/codecrafters-io/tester-utils/test_case_harness"
+)
+
func testAofFilterCommandsBeforeWrite(_ *test_case_harness.TestCaseHarness) error {
- return nil
+ return errors.New("test not implemented: AOF filter commands before write assertions are pending")
}
diff --git a/internal/test_aof_write_multiple_commands.go b/internal/test_aof_write_multiple_commands.go
--- a/internal/test_aof_write_multiple_commands.go
+++ b/internal/test_aof_write_multiple_commands.go
@@ -1,7 +1,11 @@
package internal
-import "github.com/codecrafters-io/tester-utils/test_case_harness"
+import (
+ "errors"
+ "github.com/codecrafters-io/tester-utils/test_case_harness"
+)
+
func testAofWriteMultipleCommands(_ *test_case_harness.TestCaseHarness) error {
- return nil
+ return errors.New("test not implemented: AOF write multiple commands assertions are pending")
}
diff --git a/internal/test_aof_write_single_command.go b/internal/test_aof_write_single_command.go
--- a/internal/test_aof_write_single_command.go
+++ b/internal/test_aof_write_single_command.go
@@ -1,7 +1,11 @@
package internal
-import "github.com/codecrafters-io/tester-utils/test_case_harness"
+import (
+ "errors"
+ "github.com/codecrafters-io/tester-utils/test_case_harness"
+)
+
func testAofWriteSingleCommand(_ *test_case_harness.TestCaseHarness) error {
- return nil
+ return errors.New("test not implemented: AOF write single command assertions are pending")
}
diff --git a/internal/test_rdb_config.go b/internal/test_rdb_config.go
--- a/internal/test_rdb_config.go
+++ b/internal/test_rdb_config.go
@@ -40,7 +40,7 @@
commandTestCase := test_cases.SendCommandTestCase{
Command: "CONFIG",
Args: []string{"GET", "dir"},
- Assertion: resp_assertions.NewConfigGetBulkStringValueAssertion("dir", tmpDir),
+ Assertion: resp_assertions.NewConfigGetDirResponseAssertion(tmpDir),
ShouldSkipUnreadDataCheck: false,
}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Hardcoded Docker path breaks local fixture matching
- Added normalization patterns for working directory paths ending in /internal so that both fixture and actual test output normalize to the same placeholder value, making tests work regardless of execution environment.
Or push these changes by commenting:
@cursor push 84088e6bc2
Preview (84088e6bc2)
diff --git a/docker_ci/docker-ci.sh b/docker_ci/docker-ci.sh
--- a/docker_ci/docker-ci.sh
+++ b/docker_ci/docker-ci.sh
@@ -13,12 +13,12 @@
test)
echo "🧪 Running tests using Docker container"
docker_build
- docker run --rm -it -v "$(pwd)":/home/runner/work/redis-tester/redis-tester -w /home/runner/work/redis-tester/redis-tester redis-tester make test
+ docker run --rm -it -v "$(pwd)":/app -w /app redis-tester make test
;;
record_fixtures)
echo "📝 Recording fixtures using Docker container"
docker_build
- docker run --rm -it -e CODECRAFTERS_RECORD_FIXTURES=true -v "$(pwd)":/home/runner/work/redis-tester/redis-tester -w /home/runner/work/redis-tester/redis-tester redis-tester make test
+ docker run --rm -it -e CODECRAFTERS_RECORD_FIXTURES=true -v "$(pwd)":/app -w /app redis-tester make test
;;
*)
echo "Usage:"
diff --git a/internal/stages_test.go b/internal/stages_test.go
--- a/internal/stages_test.go
+++ b/internal/stages_test.go
@@ -219,6 +219,8 @@
" tmp_dir ": {regexp.MustCompile(` /private/tmp/[^ ]+ `), regexp.MustCompile(` /tmp/[^ ]+ `)},
"$length\\r\\ntmp_dir\\r": {regexp.MustCompile(`\$\d+\\r\\n/private/tmp/[^ ]+\\r\\n`), regexp.MustCompile(`\$\d+\\r\\n/tmp/[^ ]+\\r\\n`)},
"\"tmp_dir\"": {regexp.MustCompile(`"/private/tmp/[^"]+"`), regexp.MustCompile(`"/tmp/[^"]+"`)},
+ "$length\\r\\nworkdir\\r": {regexp.MustCompile(`\$\d+\\r\\n/[^\r]+/internal\\r\\n`)},
+ "\"workdir\"": {regexp.MustCompile(`"/[^"]+/internal"`)},
"timestamp": {regexp.MustCompile(`\d{2}:\d{2}:\d{2}\.\d{3}`)},
"info_replication": {regexp.MustCompile(`"# Replication\\r\\n[^"]+"`)},
"replication_id": {regexp.MustCompile(`FULLRESYNC [A-Za-z0-9]+ 0`)},
UdeshyaDhungana
commented
Mar 26, 2026
internal/tester_definition.go
Outdated
| Slug: "sm4", | ||
| TestFunc: testRdbReadValueWithExpiry, | ||
| }, | ||
| // AOF persistence (write path) |
Contributor
Author
There was a problem hiding this comment.
I'll update this comment later
rohitpaulk
approved these changes
Mar 27, 2026
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing fixture normalization for working directory path
- Added CWD normalization patterns to normalizeTesterOutput() and updated the aof/pass fixture to use stable placeholders instead of hardcoded CI paths.
Or push these changes by commenting:
@cursor push d7b8f3c4cb
Preview (d7b8f3c4cb)
diff --git a/internal/stages_test.go b/internal/stages_test.go
--- a/internal/stages_test.go
+++ b/internal/stages_test.go
@@ -237,5 +237,17 @@
}
}
+ // Normalize the current working directory path (used by testAofConfigDefaults)
+ cwd, err := os.Getwd()
+ if err == nil && cwd != "" {
+ escapedCwd := regexp.QuoteMeta(cwd)
+ // Replace RESP bulk string containing CWD (e.g., $52\r\n/path/to/cwd\r\n)
+ respPattern := regexp.MustCompile(`\$\d+\\r\\n` + escapedCwd + `\\r\\n`)
+ testerOutput = respPattern.ReplaceAll(testerOutput, []byte(`$length\r\ncwd\r`))
+ // Replace quoted CWD in display output (e.g., "/path/to/cwd")
+ quotedPattern := regexp.MustCompile(`"` + escapedCwd + `"`)
+ testerOutput = quotedPattern.ReplaceAll(testerOutput, []byte(`"cwd"`))
+ }
+
return testerOutput
}
diff --git a/internal/test_helpers/fixtures/aof/pass b/internal/test_helpers/fixtures/aof/pass
--- a/internal/test_helpers/fixtures/aof/pass
+++ b/internal/test_helpers/fixtures/aof/pass
@@ -5,14 +5,14 @@
�[33m[tester::#UJ3] [client] �[0m�[36mConnected (port 53302 -> port 6379)�[0m
�[33m[tester::#UJ3] [client] �[0m�[94m$ redis-cli CONFIG GET dir�[0m
�[33m[tester::#UJ3] [client] �[0m�[36mSent bytes: "*3\r\n$6\r\nCONFIG\r\n$3\r\nGET\r\n$3\r\ndir\r\n"�[0m
-�[33m[tester::#UJ3] [client] �[0m�[36mReceived bytes: "*2\r\n$3\r\ndir\r\n$52\r\n/home/runner/work/redis-tester/redis-tester/internal\r\n"�[0m
+�[33m[tester::#UJ3] [client] �[0m�[36mReceived bytes: "*2\r\n$3\r\ndir\r\n$length\r\ncwd\r"�[0m
�[33m[tester::#UJ3] [client] �[0m�[36mReceived RESP array: [�[0m
�[33m[tester::#UJ3] [client] �[0m�[36m "dir",�[0m
-�[33m[tester::#UJ3] [client] �[0m�[36m "/home/runner/work/redis-tester/redis-tester/internal"�[0m
+�[33m[tester::#UJ3] [client] �[0m�[36m "cwd"�[0m
�[33m[tester::#UJ3] [client] �[0m�[36m]�[0m
�[33m[tester::#UJ3] [client] �[0m�[92m✔︎ Received [�[0m
�[33m[tester::#UJ3] [client] �[0m�[92m "dir",�[0m
-�[33m[tester::#UJ3] [client] �[0m�[92m "/home/runner/work/redis-tester/redis-tester/internal"�[0m
+�[33m[tester::#UJ3] [client] �[0m�[92m "cwd"�[0m
�[33m[tester::#UJ3] [client] �[0m�[92m]�[0m
�[33m[tester::#UJ3] [client] �[0m�[94m> CONFIG GET appendonly�[0m
�[33m[tester::#UJ3] [client] �[0m�[36mSent bytes: "*3\r\n$6\r\nCONFIG\r\n$3\r\nGET\r\n$10\r\nappendonly\r\n"�[0m…ion-cleanup-0229 Filesystem assertion cleanup
…ld-name-39a1 Panic message field name
…-values-5440 AOF test distinct values
AOF (Stages 9-10)
AOF (Stages 6-8)
AOF (Stages 3-5)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Unused
Cleanupmethod with unused parameter- Removed the unused Cleanup method and its unused test_case_harness import since AOF tests handle cleanup directly via os.RemoveAll in their teardown functions.
Or push these changes by commenting:
@cursor push c4e3703955
Preview (c4e3703955)
diff --git a/internal/aof_directory_creator.go b/internal/aof_directory_creator.go
--- a/internal/aof_directory_creator.go
+++ b/internal/aof_directory_creator.go
@@ -9,7 +9,6 @@
encoder "github.com/codecrafters-io/redis-tester/internal/resp/encoder"
value "github.com/codecrafters-io/redis-tester/internal/resp/value"
"github.com/codecrafters-io/tester-utils/logger"
- "github.com/codecrafters-io/tester-utils/test_case_harness"
"github.com/dustin/go-humanize/english"
)
@@ -121,10 +120,6 @@
return nil
}
-func (a *AofDirectoryCreator) Cleanup(stageHarness *test_case_harness.TestCaseHarness) error {
- return os.RemoveAll(filepath.Join(a.DataDirectory, a.AppendDirName))
-}
-
func (a *AofDirectoryCreator) verifyMemberValues() {
if a.DataDirectory == "" {
panic("Codecrafters Internal Error - DataDirectory cannot be empty in AofDirectoryCreator")You can send follow-ups to the cloud agent here.
The Cleanup method was never called anywhere in the codebase. AOF test files handle cleanup directly using os.RemoveAll in their teardown functions registered via stageHarness.RegisterTeardownFunc. Additionally, the stageHarness parameter in the method was unused, and the method returned error unlike RDBFileCreator.Cleanup() which returns nothing, creating an inconsistency. This also removes the now-unused test_case_harness import.
…or-cleanup-method-b7cd AofDirectoryCreator cleanup method
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Non-idiomatic Go return order swaps error position
- Changed the return signature of assertEmptyAofFileCase from (error, bool) to (bool, error) and updated the call site to match Go's convention of placing error as the last return value.
Or push these changes by commenting:
@cursor push b8b7fb6a4f
Preview (b8b7fb6a4f)
diff --git a/internal/filesystem_assertion/aof_append_only_file_assertion.go b/internal/filesystem_assertion/aof_append_only_file_assertion.go
--- a/internal/filesystem_assertion/aof_append_only_file_assertion.go
+++ b/internal/filesystem_assertion/aof_append_only_file_assertion.go
@@ -44,7 +44,7 @@
// Assert the empty case first separatly, we need not decode commands here
// Checking file contents is enough
- if err, done := a.assertEmptyAofFileCase(fileContents); done {
+ if done, err := a.assertEmptyAofFileCase(fileContents); done {
return FilesystemAssertionResult{
Logs: a.accumulatedLogs,
Err: err,
@@ -84,17 +84,17 @@
}
}
-func (a *AofAppendOnlyFileAssertion) assertEmptyAofFileCase(fileContents []byte) (error, bool) {
+func (a *AofAppendOnlyFileAssertion) assertEmptyAofFileCase(fileContents []byte) (bool, error) {
if len(a.ExpectedCommands) != 0 {
- return nil, false
+ return false, nil
}
if len(fileContents) > 0 {
- return errors.New("Expected append-only file to be empty, is not empty"), true
+ return true, errors.New("Expected append-only file to be empty, is not empty")
}
a.registerLog(NewFilesystemAssertionLog(_SUCCESS, "✔ Append-only file is empty"))
- return nil, true
+ return true, nil
}
func (a *AofAppendOnlyFileAssertion) decodeCommandsFromAppendOnlyFile(fileContents []byte) ([][]string, error) {You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 9d7e674. Configure here.
Change the return signature from (error, bool) to (bool, error) to follow Go's convention of placing the error as the last return value. Update the call site accordingly.
…rn-order-140b Idiomatic Go return order
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Medium Risk
Adds a new AOF persistence extension with multiple new test stages and filesystem/RESP decoding assertions; failures could be flaky due to concurrent, time-based filesystem polling and OS-specific path/permissions behavior.
Overview
Adds a new AOF Persistence extension (course definition + tester wiring) with 10 new stages covering AOF option defaults/flags, append-only directory/file/manifest creation, writing commands to AOF (including filtering non-mutating commands), and replaying commands on startup.
Introduces new testing infrastructure to validate filesystem side effects:
FilesystemAsserter(concurrent, retry-until-timeout runner), assertions for directory existence, manifest contents, and decoding/validating AOF command streams (new RESP decoder helper for AOF concatenated arrays). Also generalizes theCONFIG GETassertion intoConfigGetBulkStringValueAssertion, updating existing RDB config tests accordingly.Updates local tooling to run AOF stages (
make test_aof_with_redis, included intest_all_with_redis) and adjusts Docker CI mount paths; adds new Go dependencies for shell escaping/quoting used in assertion error messages.Reviewed by Cursor Bugbot for commit 8ff2733. Bugbot is set up for automated code reviews on this repo. Configure here.