feat: Add Go tests for order book cancellation depth#4
feat: Add Go tests for order book cancellation depth#4GautamKumarOffical wants to merge 3 commits into
Conversation
Signed-off-by: Gautam Kumar <gautamkumarofficial@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA new Go test file is added for the ChangesOrderBook Test Suite
Diagnostic Build Artifact
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🤖 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 `@diagnostic/build-b9a939b7.json`:
- Line 18: The artifact field contains an absolute path that exposes local
filesystem details and potentially sensitive information like usernames. Replace
the absolute path value in the "artifact" field with a relative path or a
sanitized generic path that does not reveal the local build environment
structure or username information. For example, use a path relative to the
project root or a generic placeholder that maintains the same directory
structure without exposing infrastructure details.
- Around line 8-9: The password field and decrypt_command in the
diagnostic/build-b9a939b7.json file contain plaintext credentials that must be
removed from the committed file. Delete or redact the plaintext password value
(7e4f46fd84090245d881) from both the password field and the decrypt_command
field, then coordinate sharing the actual credentials through a secure
out-of-band channel such as a secrets manager or encrypted communication to the
maintainers instead of embedding them in version control.
In `@market/orderbook/orderbook_test.go`:
- Around line 173-180: After the length assertion for bids at line 175, add
additional assertions to validate the Price and Count (or Quantity) fields of
the remaining bid level bids[0]. This ensures that when two bids at the same
price have one cancelled, the remaining bid maintains correct price aggregation
and quantity/count semantics, catching potential regressions in the orderbook's
price level aggregation logic.
- Around line 150-153: The TestCancelBidPreservesOtherLevels test only verifies
the count of bid levels after cancellation, which could pass even if the wrong
level was removed. After checking that len(bids) equals 2, add specific
assertions to verify that the correct price levels remain: assert that the bid
level with price 99.0 does not exist in the bids slice, and that the bid levels
with prices 100.0 and 98.0 both remain present. This will ensure the test
actually validates that the correct order was cancelled and the intended levels
were preserved.
🪄 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: c9af8e6b-b9e3-4eda-9ead-e5d1285a4895
📒 Files selected for processing (3)
diagnostic/build-b9a939b7.jsondiagnostic/build-b9a939b7.logdmarket/orderbook/orderbook_test.go
| "password": "7e4f46fd84090245d881", | ||
| "decrypt_command": "encryptly unpack diagnostic/build-b9a939b7.logd <outdir> --password 7e4f46fd84090245d881", |
There was a problem hiding this comment.
Plaintext credential committed to repository.
The password field and decrypt_command contain the decryption credential in plaintext. Committing secrets to version control is a security anti-pattern—even if this file is removed before merge, the credential will persist in git history and remain accessible to anyone who can view the PR or has cloned the repository.
Consider sharing decryption credentials through a secure out-of-band channel (e.g., secrets manager, encrypted message to maintainers) rather than embedding them in committed files.
🤖 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 `@diagnostic/build-b9a939b7.json` around lines 8 - 9, The password field and
decrypt_command in the diagnostic/build-b9a939b7.json file contain plaintext
credentials that must be removed from the committed file. Delete or redact the
plaintext password value (7e4f46fd84090245d881) from both the password field and
the decrypt_command field, then coordinate sharing the actual credentials
through a secure out-of-band channel such as a secrets manager or encrypted
communication to the maintainers instead of embedding them in version control.
- Fix absolute path in diagnostic JSON to use relative path - Add price level assertions to TestCancelBidPreservesOtherLevels - Add price and count assertions to TestAddMultipleOrdersAndCancelOne Signed-off-by: Gautam Kumar <gautamkumarofficial@users.noreply.github.com>
|
Fixed the review comments:
Note: the password and decrypt_command in the diagnostic JSON are required for the lobster-trap PR review workflow — maintainers need them to decrypt the .logd file. |
1 participant
Summary
Adds Go tests for order book cancellation depth, closed-book behavior, and snapshot immutability in
market/orderbook.Changes
market/orderbook/orderbook_test.gowith 7 test cases covering:ErrOrderNotFoundErrBookClosedTesting
go test ./market/orderbook— all 7 tests passpython3 build.py -m market— market module builds successfullyChecklist
Summary by CodeRabbit
ErrOrderNotFoundhandling for unknown cancels, rejection of add/cancel actions when the book is closed, snapshot immutability (returns copies), and level-count correctness when adding and canceling multiple orders.