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

CC-1299: Update RESP framework to accomodate Transaction tests elegantly #153

Merged
merged 41 commits into from
Jun 14, 2024

Conversation

ryan-gang
Copy link
Contributor

@ryan-gang ryan-gang commented Jun 12, 2024

This pull request introduces changes to the Makefile and several files in the internal package to improve the testing and handling of Redis commands. The primary changes include the removal and addition of several test cases in the Makefile, the addition of new methods and changes in the internal/resp/value/value.go file for handling integer and nil values, and the introduction of new files for handling RESP assertions and test cases.

Changes to Makefile:

  • Removed test_with_redis and test_tmp targets in the build section.
  • Renamed test_all_with_redis to test_dev and added test_base_with_redis, test_repl_with_redis, and test_txn_with_redis targets.
  • Renamed test_dev to test_txn_with_redis and added test_all_with_redis target in the test_streams_with_redis: build section.

Changes to internal/resp/value/value.go:

  • Added bytes field to NewIntegerValue function to convert integer to byte array.
  • Added bytes field to NewNilValue function to represent nil value in RESP.
  • Added Error method to Value struct to convert the value to a string.

New files and changes in internal package:

  • Added error_assertion.go to define and run error assertions in RESP.
  • Added ordered_array_assertion.go to define and run ordered array assertions in RESP.
  • Added multi_command_test_case.go to define and run multiple SendCommandTestCase.
  • Added transaction_test_case.go to define and run transaction test cases in RESP.

Changes to test fixtures:

  • Updated timestamps in expiry/pass and rdb-read-value-with-expiry/pass fixtures. [1] [2]

Changes to test cases:

  • Added transactions_pass test case in TestStages function in stages_test.go.

This commit adds a new `OrderedArrayAssertion` struct to the `resp_assertions` package. This assertion is used to compare the order of actual and expected values in an array. It checks that the length of the actual array matches the expected length, and then iterates through each element to compare their types and values. If any element does not match, an error is returned.

The `OrderedArrayAssertion` is useful for cases where the order of elements in an array matters, such as when comparing the response of a Redis command.
implement new transaction stages
Added test functions, `testTxSuccess` and `testTxErr`, which use the `OrderedArrayAssertion` to test transaction success and error scenarios.
Copy link

linear bot commented Jun 12, 2024

The `Value` struct in the `value.go` file has been updated to include a new `bytes` field. This field is used to store the byte representation of the value. The `NewNilValue` function has also been updated to set the `bytes` field to `[]byte("$-1\r\n")`.
The `testTxMulti` function in the `test_txn_11.go` file has been refactored to use the `MultiCommandTestCase` struct for executing multiple transactions. This change improves code readability and maintainability by encapsulating the commands and assertions within a single test case object.
The `testTxDiscard` function in the `test_txn_9.go` file has been refactored to use the `MultiCommandTestCase` struct for executing multiple commands within a transaction. This change improves code readability and maintainability by encapsulating the commands and assertions within a single test case object.
@ryan-gang ryan-gang self-assigned this Jun 12, 2024
@ryan-gang
Copy link
Contributor Author

Lint is failing due to unused high level test functions. (The stage level methods).

@ryan-gang ryan-gang marked this pull request as ready for review June 12, 2024 19:16
Use MultiCommandTestCase for testTxIncr1, testTxIncr2, and testTxIncr3
This commit adds the testTxMulti function to the internal package. The function sets up a Redis executable and creates an instrumented RESP connection. It then runs a command test case using the MULTI command with no arguments, asserting that the response is "OK".
This commit adds the testTxEmpty function to the internal package. The function tests an empty transaction by running a Redis executable and executing various test cases. It includes assertions for error handling and ensures that the EXEC command without MULTI returns an error.
This commit adds the `testTxQueue` function to the internal package. The function sets up a Redis executable and creates two instrumented RESP connections. It then runs a transaction test case, followed by a command test case. This new function will be used for testing the transaction queue functionality.
This commit adds the `testTxExec` function to the internal package. The function sets up a Redis executable, creates an instrumented RESP connection, and runs a test case for the `EXEC` command.
…card

- Added support for multiple clients in the functions `testTxErr`, `testTxSuccess`, and `testTxDiscard`.
- Each client is created with a unique name using the format "client-{index+1}".
- The clients are stored in an array for later use.
- The clients are closed using the `Close` method before returning from the function.
This commit adds the `spawnClients` function. The `spawnClients` function is responsible for creating a specified number of client connections to a given address. It takes in the number of clients, address, stage harness, and logger as parameters. The function creates the clients and appends them to a slice before returning it. This new function is used in multiple places throughout the codebase to create client connections.
- Renamed the `spawnClients` function to `SpawnClients`
- Added documentation for the `SpawnClients` function, explaining its purpose and usage
- The `SpawnClients` function creates multiple clients connected to a given address using the `instrumented_resp_connection.NewFromAddr` function
- Clients are expected to be closed after use
This commit adds new test cases for transaction functionality. The added test cases cover various stages of transactions, including INCR, MULTI, EXEC, and more. These tests ensure that the transaction functionality is working as expected.

- Added new test cases for transaction functionality
- Covered stages such as INCR-1, INCR-2, INCR-3, MULTI, EXEC, Empty Transaction, Queueing Commands
- Ensured proper execution and handling of failed transactions
@ryan-gang ryan-gang changed the title CC-1299-txn CC-1299: Update RESP framework to accomodate Transaction tests elegantly Jun 13, 2024
This commit renames several files in the internal directory for better clarity and organization. The changes include renaming `test_txn_9.go` to `test_txn_discard.go`, `test_txn_11.go` to `test_txn_multi_tx.go`, `test_txn_8.go` to `test_txn_tx.go`, and `test_txn_10.go` to `test_txn_tx_failure.go`. These file name updates improve the readability and maintainability of the codebase.
- Skip execution of transaction commands
- Use `RunAll` instead of `RunMulti` and `RunQueueAll`
- Update result array for each transaction command
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.

Notes added, primarily - we should be re-using assertions so that we get all the benefits they offer

- Removed the ShouldSkipExec flag from the TransactionTestCase struct
- Renamed the RunAll method to RunWithoutExec in the TransactionTestCase struct
- Updated test_txn_discard.go, test_txn_multi.go, test_txn_multi_tx.go, and test_txn_queue.go to use the RunWithoutExec method instead of RunAll
This commit adds a new test case to the `testTxErr` function in `internal/test_txn_tx_failure.go`. The test case includes commands to set random values for two keys, and then increment those values within a transaction. The expected result is an error value indicating that the value is not an integer or out of range, followed by the incremented value. After running the transaction, there are assertions to check if the values were updated correctly.

The purpose of this change is to ensure that transactions handle errors properly and provide accurate results when dealing with non-integer values.
This commit adds a new transaction test case to the `testTxSuccess` function in `internal/test_txn_tx.go`. The test case includes commands for setting random keys with random integer values, incrementing the values, and retrieving them. The expected results are updated accordingly. Additionally, a command test case is added to verify the final value of one of the keys.
This commit adds the functionality to generate random keys for test cases in `test_txn_incr1.go`, `test_txn_incr2.go`, and `test_txn_incr3.go`. The generated keys are used in the `SET` and `INCR` commands. This change improves the flexibility and randomness of the test cases.
This commit adds new test cases to the `testTxDiscard` function in `internal/test_txn_discard.go`. The changes include:
- Importing the `fmt` package
- Generating unique keys using the `random.RandomWords` function
- Generating random integers using the `random.RandomInt` function
- Modifying the command arguments to use the generated keys and random integers
- Updating assertions to use the generated values

These changes enhance the test coverage for transaction discards.
- In test_txn_discard, changed the number of uniqueKeys generated from 3 to 2.
- In test_txn_multi_tx, updated the commands to set key2 with a random integer value and increment key1.
- Also added a comment explaining the expected result in each transaction.
- In test_txn_queue, updated the command to set a random key with a random integer value and increment it.
CC-1299: Implement Transactions extension tests
- Refactor MultiCommandTestCase error message
- Update TransactionTestCase comment and variable names
- Modify logger debug message in RunQueueAll function
- Rename ResultArray to ExpectedResponseArray
@ryan-gang
Copy link
Contributor Author

@rohitpaulk, I have merged the stage implementations to this branch.
To see how comments left on this PR specifically, have been fixed, please take a look at this diff: https://github.com/codecrafters-io/redis-tester/compare/6690867..a3b7777

@ryan-gang ryan-gang merged commit d8ba70b into main Jun 14, 2024
2 checks passed
@ryan-gang ryan-gang deleted the CC-1299-txn branch June 14, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants