[CLOB-636] - update e2e stateful cancellations tests #750
Conversation
CLOB-636 Write end-to-end tests for order cancellations
We should write end-to-end tests that have better coverage of order cancellations, at a minimum we should cover the following cases:
|
WalkthroughThe recent changes involve the addition of a new utility function for comparing slices, enhancements to the testing utilities, and updates to the blockchain application tests. A new function Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/lib/slices.go (1 hunks)
- protocol/testutil/app/app.go (2 hunks)
- protocol/x/clob/e2e/app_test.go (1 hunks)
- protocol/x/clob/e2e/long_term_orders_test.go (8 hunks)
Files skipped from review due to trivial changes (1)
- protocol/testutil/app/app.go
Additional comments: 6
protocol/lib/slices.go (1)
- 1-13: The function
SlicesAreEqualis correctly implemented. It first checks if the lengths of the two slices are equal. If they are not, it immediately returns false. Then it iterates over the elements of the first slice and checks if each element is equal to the corresponding element in the second slice. If it finds any pair of elements that are not equal, it immediately returns false. If it doesn't find any unequal pairs, it returns true after the loop, indicating that the slices are equal. This is a good and efficient way to compare slices.protocol/x/clob/e2e/long_term_orders_test.go (4)
10-11: New imports have been added. Ensure that these packages are used in the code and that they are necessary for the functionality of the code.
74-78: A new struct
checkResultshas been introduced to store the expected results of the tests. This is a good practice as it improves the readability and maintainability of the code.80-147: A new test function
TestCancelFullyFilledStatefulOrderInSameBlockItIsFilledhas been added. This function tests the scenario where a stateful cancellation fails because the order was fully filled in the same block. The test function seems to be well-structured and covers the necessary assertions.250-285: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [149-306]
Existing test functions have been modified to use the new struct
checkResultsfor storing and checking expected test results. This is a good practice as it improves the readability and maintainability of the code.protocol/x/clob/e2e/app_test.go (1)
- 179-188: The new variable
PlaceOrder_Bob_Num0_Id0_Clob0_Sell4_Price10_GTB20is correctly initialized with specific order values. Ensure that this new order is used correctly in the tests where it is needed.
jakob-dydx
force-pushed
the
jakob-dydx/stateful-cancellation-e2e
branch
from
7b7502d to
23cf401
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/lib/slices.go (1 hunks)
- protocol/testutil/app/app.go (2 hunks)
- protocol/x/clob/e2e/app_test.go (1 hunks)
- protocol/x/clob/e2e/long_term_orders_test.go (8 hunks)
Files skipped from review due to trivial changes (1)
- protocol/testutil/app/app.go
Additional comments: 5
protocol/lib/slices.go (1)
- 1-13: The function
SlicesAreEqualis correctly implemented. It first checks if the lengths of the two slices are equal. If they are not, it immediately returns false. Then it iterates over the elements of the first slice and checks if each element is equal to the corresponding element in the second slice. If it finds any pair of elements that are not equal, it immediately returns false. If it doesn't find any unequal pairs, it returns true after the loop, indicating that the slices are equal. This is a good and efficient way to compare slices.protocol/x/clob/e2e/long_term_orders_test.go (3)
10-11: The new imports are not used in the provided hunks. Ensure they are used elsewhere in the file or remove them to avoid unnecessary imports.
74-141: The new test function
TestCancelFullyFilledStatefulOrderInSameBlockItIsFilledis well-structured and follows the Arrange-Act-Assert pattern. It tests the scenario where a stateful cancellation fails because the order was fully filled in the same block. The test case is clear and the assertions are well defined.143-150: The new struct
checkResultsis used to store and check expected test results. This is a good practice as it improves readability and maintainability of the test cases.protocol/x/clob/e2e/app_test.go (1)
- 179-188: The new variable
PlaceOrder_Bob_Num0_Id0_Clob0_Sell4_Price10_GTB20is correctly initialized with a new order. Ensure that this new order is used correctly in the tests where it is needed.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/e2e/long_term_orders_test.go (8 hunks)
Additional comments: 3
protocol/x/clob/e2e/long_term_orders_test.go (3)
74-141: The new test case
TestCancelFullyFilledStatefulOrderInSameBlockItIsFilledis well written and covers a specific scenario. It checks the cancellation of an order that was fully filled in the same block. The test case is clear and easy to understand.143-147: The
checkResultsstruct is used to store the expected results of the test cases. It includes the order id and a boolean indicating whether the order exists in the state.249-284: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [149-303]
The test cases in the
TestCancelStatefulOrderfunction have been updated to use thecheckResultsstruct for storing and checking the expected results. This is a good practice as it improves the readability and maintainability of the test cases.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/e2e/long_term_orders_test.go (8 hunks)
Additional comments: 2
protocol/x/clob/e2e/long_term_orders_test.go (2)
10-11: The imports are indeed used in the code. No changes needed.
74-144: The new test function
TestCancelFullyFilledStatefulOrderInSameBlockItIsFilledis well-structured and covers a specific scenario. It checks the case where a stateful cancellation fails because the order was fully filled in the same block. The test function is clear and easy to understand.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/e2e/long_term_orders_test.go (8 hunks)
Additional comments: 2
protocol/x/clob/e2e/long_term_orders_test.go (2)
10-11: The imports are indeed used in the code. No changes needed.
74-145: The new test function
TestCancelFullyFilledStatefulOrderInSameBlockItIsFilledis well-structured and covers a specific scenario. It checks the scenario where a stateful cancellation fails because the order was fully filled in the same block. The test case is well-structured and covers all the necessary assertions.
| type checkResults struct { | ||
| orderId clobtypes.OrderId | ||
| existsInState bool | ||
| } |
There was a problem hiding this comment.
The checkResults struct is a good addition to the test cases. It improves readability and maintainability. However, it might be beneficial to include the expected fill amount and existence in the fill in the struct. This would allow for more flexible and comprehensive test cases.
- type checkResults struct {
- orderId clobtypes.OrderId
- existsInState bool
- }
+ type checkResults struct {
+ orderId clobtypes.OrderId
+ existsInState bool
+ existsInFill bool
+ fillAmount uint64
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| type checkResults struct { | |
| orderId clobtypes.OrderId | |
| existsInState bool | |
| } | |
| type checkResults struct { | |
| orderId clobtypes.OrderId | |
| existsInState bool | |
| existsInFill bool | |
| fillAmount uint64 | |
| } |
| _, exists := tApp.App.ClobKeeper.GetLongTermOrderPlacement(ctx, tc.expectations.orderId) | ||
| require.Equal(t, exists, tc.expectations.existsInState) | ||
| exists, fillAmount, _ := tApp.App.ClobKeeper.GetOrderFillAmount( | ||
| ctx, | ||
| tc.checkCancelledPlaceOrder.Order.OrderId, | ||
| tc.expectations.orderId, |
There was a problem hiding this comment.
The test case always expects the order fill amount to be 0 and its existence to be false. This might not be the case for all tests. Consider updating the checkResults struct to include these expectations and use them in the assertions.
- type checkResults struct {
- orderId clobtypes.OrderId
- existsInState bool
- }
+ type checkResults struct {
+ orderId clobtypes.OrderId
+ existsInState bool
+ existsInFill bool
+ fillAmount uint64
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| _, exists := tApp.App.ClobKeeper.GetLongTermOrderPlacement(ctx, tc.expectations.orderId) | |
| require.Equal(t, exists, tc.expectations.existsInState) | |
| exists, fillAmount, _ := tApp.App.ClobKeeper.GetOrderFillAmount( | |
| ctx, | |
| tc.checkCancelledPlaceOrder.Order.OrderId, | |
| tc.expectations.orderId, | |
| type checkResults struct { | |
| orderId clobtypes.OrderId | |
| existsInState bool | |
| existsInFill bool | |
| fillAmount uint64 | |
| } | |
| check := func(tc testCase, results checkResults) { | |
| _, exists := tApp.App.ClobKeeper.GetLongTermOrderPlacement(ctx, results.orderId) | |
| require.Equal(t, results.existsInState, exists) | |
| exists, fillAmount, _ := tApp.App.ClobKeeper.GetOrderFillAmount( | |
| ctx, | |
| results.orderId, | |
| ) | |
| require.Equal(t, results.existsInFill, exists) | |
| require.Equal(t, results.fillAmount, fillAmount) | |
| } | |
| results := checkResults{ | |
| orderId: tc.expectations.orderId, | |
| existsInState: tc.expectations.existsInState, | |
| existsInFill: false, | |
| fillAmount: 0, | |
| } | |
| check(tc, results) |
No description provided.