fix(#2): [$40 BOUNTY] [Go] Add order book cancellation depth tests#11
fix(#2): [$40 BOUNTY] [Go] Add order book cancellation depth tests#11Notime02 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughA new test file ChangesOrderbook Test Suite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@market/orderbook/orderbook_test.go`:
- Line 12: The CancelOrder method calls at both locations are not checking the
returned error before proceeding with state assertions. Capture the error
returned from each CancelOrder call, assert that it is nil using a test
assertion (fail the test immediately if an error occurred), and only then
proceed with the subsequent state validation. This ensures that any cancellation
failures are caught before misleading assertions on the orderbook state.
- Line 11: The AddBid method is being used inconsistently throughout the test
file. On line 11, orderID is assigned directly from AddBid as if it returns only
an order ID, but on lines 41-44 it appears to be treated as returning an error.
Check the actual return signature of the AddBid method in the orderbook
implementation and then adjust all usages in the test file to match the correct
signature consistently. If AddBid returns both an order ID and an error, update
line 11 to handle both return values. If it returns only an order ID or only an
error, fix lines 41-44 accordingly to match that signature.
- Around line 14-15: The tests are accessing ob.bids and ob.asks inconsistently:
using orderID as a key in some assertions (lines 14, 24) and integer indices in
others (line 55), which is incompatible since Go maps and slices cannot use the
same access pattern. Additionally, the test assertions are checking the wrong
contract - instead of verifying that an order exists or doesn't exist directly
in ob.bids, verify the actual implementation behavior by checking that the order
is removed from ob.orders and that price levels in the bid/ask data structures
are updated accordingly. Update all assertions to consistently validate the
correct internal state changes that result from order cancellation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4acd47a-f194-4409-939f-2b88d3b082bd
📒 Files selected for processing (1)
market/orderbook/orderbook_test.go
|
|
||
| func TestCancelBid(t *testing.T) { | ||
| ob := NewOrderBook() | ||
| orderID := ob.AddBid(100, 1) |
There was a problem hiding this comment.
AddBid is asserted with incompatible return types (compile blocker).
Line 11 treats AddBid as returning an order ID, but Line 41 treats the same call as returning an error. Both cannot be true for one method signature, so this test file won’t compile as written.
Also applies to: 41-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@market/orderbook/orderbook_test.go` at line 11, The AddBid method is being
used inconsistently throughout the test file. On line 11, orderID is assigned
directly from AddBid as if it returns only an order ID, but on lines 41-44 it
appears to be treated as returning an error. Check the actual return signature
of the AddBid method in the orderbook implementation and then adjust all usages
in the test file to match the correct signature consistently. If AddBid returns
both an order ID and an error, update line 11 to handle both return values. If
it returns only an order ID or only an error, fix lines 41-44 accordingly to
match that signature.
| func TestCancelBid(t *testing.T) { | ||
| ob := NewOrderBook() | ||
| orderID := ob.AddBid(100, 1) | ||
| ob.CancelOrder(orderID) |
There was a problem hiding this comment.
Assert CancelOrder success before checking state.
Both tests ignore the returned error from CancelOrder; a failed cancel can produce misleading state assertions. Check err first and fail fast.
Also applies to: 22-22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@market/orderbook/orderbook_test.go` at line 12, The CancelOrder method calls
at both locations are not checking the returned error before proceeding with
state assertions. Capture the error returned from each CancelOrder call, assert
that it is nil using a test assertion (fail the test immediately if an error
occurred), and only then proceed with the subsequent state validation. This
ensures that any cancellation failures are caught before misleading assertions
on the orderbook state.
| if _, exists := ob.bids[orderID]; exists { | ||
| t.Fatalf("expected bid to be removed, but it still exists") |
There was a problem hiding this comment.
ob.bids/ob.asks are accessed with incompatible key/index assumptions.
The tests index ob.bids by orderID (Lines 14/24) and by integer index (Line 55). A Go map/slice cannot support both in one type. Also, cancellation in the provided implementation removes order IDs from ob.orders and price levels from bid/ask levels, so these assertions are validating the wrong internal contract.
Also applies to: 24-25, 55-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@market/orderbook/orderbook_test.go` around lines 14 - 15, The tests are
accessing ob.bids and ob.asks inconsistently: using orderID as a key in some
assertions (lines 14, 24) and integer indices in others (line 55), which is
incompatible since Go maps and slices cannot use the same access pattern.
Additionally, the test assertions are checking the wrong contract - instead of
verifying that an order exists or doesn't exist directly in ob.bids, verify the
actual implementation behavior by checking that the order is removed from
ob.orders and that price levels in the bid/ask data structures are updated
accordingly. Update all assertions to consistently validate the correct internal
state changes that result from order cancellation.
1 participant
Fix
Added unit tests for order cancellation and snapshot behavior in the order book package as specified in the issue description.
Otomatik oluşturulmuştur.
Summary by CodeRabbit