Add order book cancellation depth tests#10
Open
Samarth1306w wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a Python offline API contract test suite driven by ChangesPython Backend API Contract Test Infrastructure
OrderBook Test Suite
Android OS Normalization and Diagnostic Artifact
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 1
🤖 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`:
- Around line 179-180: The AddOrder method calls for orderBid and orderAsk are
discarding their return errors using blank identifiers, which can cause panic
when accessing snapshot.Bids[0] and snapshot.Asks[0] if the setup failed.
Instead of using _, _ = ob.AddOrder(...), explicitly check the errors returned
from both AddOrder calls and add assertions or error handling to ensure both
orders were successfully added before proceeding to index into the snapshot
data.
🪄 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: 9eeca3b3-7fc4-488a-a185-fb1a0f445868
📒 Files selected for processing (5)
build.pydiagnostic/build-2b54872c.jsondiagnostic/build-2b54872c.logdmarket/orderbook/orderbook_test.gotools/encryptly/linux-arm64/encryptly
…, and regenerate size-compliant diagnostics
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@backend/api_contract.py`:
- Around line 194-195: The _requires_authorization function currently uses an
unreliable heuristic by checking if the operation path starts with "/auth/" to
determine authorization requirements. Instead, modify this function to derive
authorization requirements from the OpenAPI security specification by checking
the security field on the ApiOperation object (which should contain the
operation's security definition from the OpenAPI spec), falling back to any
global security settings if needed. This ensures authorization is determined by
the actual OpenAPI contract rather than URL pattern matching, which will prevent
misclassification of protected and public endpoints.
- Around line 164-166: The object payload validation in the code block checking
`isinstance(val, dict)` is too restrictive and will incorrectly flag valid
Mapping implementations as malformed. Replace the `isinstance(val, dict)` check
with a check using `collections.abc.Mapping` to properly validate any mapping
type instead of only accepting dict instances. This aligns with the contract
that accepts `Mapping[str, Any]` for payload validation.
In `@diagnostic/build-e84e063f.json`:
- Around line 8-9: Remove the plaintext password credential and decrypt_command
fields from the diagnostic/build-e84e063f.json file. Delete the password field
(containing "250c1ab15dd61da344f7") and the decrypt_command field that
references it. Instead, communicate the decryption password to maintainers
through a secure channel outside of version control to avoid storing credentials
in the repository.
In `@tests/backend_api/test_openapi_contract.py`:
- Around line 79-84: The
test_internal_error_path_is_mocked_without_external_dependencies function and
the similar test at lines 104-109 both hardcode endpoint paths like
"/auth/login" and "/users" along with fixed expected status codes like 200.
Instead of using hardcoded values, select documented operations from the
api_operations collection and use the operation.success_statuses property to
determine expected status codes. This makes the tests resilient to OpenAPI spec
changes. Update both affected tests to dynamically reference operations from
api_operations rather than hardcoding the endpoint paths and status assertions.
- Around line 90-101: The test in
test_edge_case_payloads_are_accepted_by_contract_mock is using hard-coded field
names (email, password, mfa_code, client_fingerprint) that may not match the
selected operation's actual schema. Instead of creating a fixed payload
dictionary, retrieve the valid payload baseline for the selected operation using
valid_payloads[(operation.method, operation.path)], make a copy of it, and then
apply edge-case mutations only to the keys that exist in that baseline payload
(such as replacing values with unicode strings or extremely long strings). This
makes the test schema-aware and prevents failures due to missing or unexpected
fields.
🪄 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: 382b3566-cbda-411b-ba45-414fd5633066
📒 Files selected for processing (15)
Makefilebackend/__init__.pybackend/api_contract.pydiagnostic/build-00000000.jsondiagnostic/build-00000000.logddiagnostic/build-e84e063f.jsondiagnostic/build-e84e063f.logdmarket/orderbook/orderbook_test.gopytest.inirequirements-dev.txttests/backend_api/README.mdtests/backend_api/test_openapi_contract.pytests/conftest.pytools/encryptly/linux-arm64/encryptlytools/encryptly/macos-x64/encryptly
💤 Files with no reviewable changes (2)
- diagnostic/build-00000000.json
- diagnostic/build-00000000.logd
✅ Files skipped from review due to trivial changes (4)
- pytest.ini
- backend/init.py
- requirements-dev.txt
- tests/backend_api/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- market/orderbook/orderbook_test.go
Comment on lines
+164
to
+166
| elif expected_type == "object": | ||
| if not isinstance(val, dict): | ||
| is_malformed = True |
There was a problem hiding this comment.
Use Mapping for object payload validation to match the request contract.
At Line 164-Line 166, object fields are rejected unless they are dict. Since payload is a Mapping[str, Any], valid mapping implementations can be falsely flagged as malformed.
Proposed fix
- elif expected_type == "object":
- if not isinstance(val, dict):
+ elif expected_type == "object":
+ if not isinstance(val, Mapping):
is_malformed = True🤖 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 `@backend/api_contract.py` around lines 164 - 166, The object payload
validation in the code block checking `isinstance(val, dict)` is too restrictive
and will incorrectly flag valid Mapping implementations as malformed. Replace
the `isinstance(val, dict)` check with a check using `collections.abc.Mapping`
to properly validate any mapping type instead of only accepting dict instances.
This aligns with the contract that accepts `Mapping[str, Any]` for payload
validation.
Comment on lines
+194
to
+195
| def _requires_authorization(operation: ApiOperation) -> bool: | ||
| return not operation.path.startswith("/auth/") |
There was a problem hiding this comment.
Derive authorization requirements from OpenAPI security, not URL prefix heuristics.
At Line 194-Line 195, auth is inferred from not operation.path.startswith("/auth/"). This can misclassify protected/public endpoints and make contract tests pass with incorrect auth behavior. The auth requirement should come from per-operation (or global) OpenAPI security.
Suggested direction
`@dataclass`(frozen=True)
class ApiOperation:
@@
parameters: tuple[str, ...]
+ requires_auth: bool
@@
- return ApiOperation(
+ op_security = operation.get("security", spec.get("security"))
+ requires_auth = bool(op_security) # [] => explicitly no auth
+ return ApiOperation(
method=method.upper(),
path=path,
@@
required_fields=required_fields,
parameters=parameters,
+ requires_auth=requires_auth,
)
@@
`@staticmethod`
def _requires_authorization(operation: ApiOperation) -> bool:
- return not operation.path.startswith("/auth/")
+ return operation.requires_authAlso applies to: 198-219
🤖 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 `@backend/api_contract.py` around lines 194 - 195, The _requires_authorization
function currently uses an unreliable heuristic by checking if the operation
path starts with "/auth/" to determine authorization requirements. Instead,
modify this function to derive authorization requirements from the OpenAPI
security specification by checking the security field on the ApiOperation object
(which should contain the operation's security definition from the OpenAPI
spec), falling back to any global security settings if needed. This ensures
authorization is determined by the actual OpenAPI contract rather than URL
pattern matching, which will prevent misclassification of protected and public
endpoints.
Comment on lines
+8
to
+9
| "password": "250c1ab15dd61da344f7", | ||
| "decrypt_command": "encryptly unpack diagnostic/build-e84e063f.logd <outdir> --password 250c1ab15dd61da344f7", |
There was a problem hiding this comment.
Remove plaintext password from diagnostic metadata file.
The file contains a plaintext encryption password (line 8) and includes it in the decrypt command (line 9). Storing credentials in version control creates lasting exposure risks, even for temporary diagnostic artifacts. This defeats the purpose of encrypting the log.
Recommended fixes:
- Remove the
passwordanddecrypt_commandfields from this JSON file. - Communicate the decryption password to maintainers via a secure channel (e.g., Slack, email, GitHub secret).
- If the diagnostic file must be committed, do not include plaintext credentials in the repository.
Alternatively, do not commit diagnostic artifacts to version control at all; share them out-of-band.
🧰 Tools
🪛 Betterleaks (1.5.0)
[high] 8-8: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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-e84e063f.json` around lines 8 - 9, Remove the plaintext
password credential and decrypt_command fields from the
diagnostic/build-e84e063f.json file. Delete the password field (containing
"250c1ab15dd61da344f7") and the decrypt_command field that references it.
Instead, communicate the decryption password to maintainers through a secure
channel outside of version control to avoid storing credentials in the
repository.
Comment on lines
+79
to
+84
| def test_internal_error_path_is_mocked_without_external_dependencies(api_client): | ||
| response = api_client.request( | ||
| "POST", | ||
| "/auth/login", | ||
| payload={"__force_internal_error__": True}, | ||
| ) |
There was a problem hiding this comment.
Avoid hardcoded endpoint/status assumptions in contract-driven tests.
At Line 79-Line 84 and Line 104-Line 109, tests assume /auth/login, /users, and 200. These can break when the OpenAPI spec changes, even if the mock is correct. Select documented operations from api_operations and assert against operation.success_statuses instead of fixed paths/status.
Proposed fix sketch
-def test_internal_error_path_is_mocked_without_external_dependencies(api_client):
- response = api_client.request(
- "POST",
- "/auth/login",
- payload={"__force_internal_error__": True},
- )
+def test_internal_error_path_is_mocked_without_external_dependencies(api_client, api_operations, auth_token):
+ operation = next(op for op in api_operations if op.method == "POST")
+ token = None if operation.path.startswith("/auth/") else auth_token
+ response = api_client.request(
+ operation.method,
+ operation.path,
+ payload={"__force_internal_error__": True},
+ token=token,
+ )
@@
-def test_async_request_wrapper(api_client, auth_token):
- response = asyncio.run(api_client.request_async("GET", "/users", token=auth_token))
-
- assert response.status_code == 200
- assert response.body["path"] == "/users"
+def test_async_request_wrapper(api_client, api_operations, auth_token):
+ operation = next(op for op in api_operations if op.method == "GET")
+ token = None if operation.path.startswith("/auth/") else auth_token
+ response = asyncio.run(api_client.request_async(operation.method, operation.path, token=token))
+
+ assert response.status_code in operation.success_statuses
+ assert response.body["path"] == operation.pathAlso applies to: 104-109
🤖 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 `@tests/backend_api/test_openapi_contract.py` around lines 79 - 84, The
test_internal_error_path_is_mocked_without_external_dependencies function and
the similar test at lines 104-109 both hardcode endpoint paths like
"/auth/login" and "/users" along with fixed expected status codes like 200.
Instead of using hardcoded values, select documented operations from the
api_operations collection and use the operation.success_statuses property to
determine expected status codes. This makes the tests resilient to OpenAPI spec
changes. Update both affected tests to dynamically reference operations from
api_operations rather than hardcoding the endpoint paths and status assertions.
Comment on lines
+90
to
+101
| def test_edge_case_payloads_are_accepted_by_contract_mock(api_client, api_operations, auth_token): | ||
| operation = next(operation for operation in api_operations if operation.method == "POST") | ||
| payload = { | ||
| "email": "unicode-\\u2603@example.com", | ||
| "password": "x" * 4096, | ||
| "mfa_code": "", | ||
| "client_fingerprint": "edge-case", | ||
| } | ||
|
|
||
| response = api_client.request(operation.method, operation.path, payload=payload, token=auth_token) | ||
|
|
||
| assert response.status_code in operation.success_statuses |
There was a problem hiding this comment.
Build edge-case payloads from the selected operation’s valid payload baseline.
At Line 90-Line 97, the test picks the first POST operation but sends fixed fields (email, password, etc.). This is operation/schema-dependent and can fail unrelated to edge-case handling. Start from valid_payloads[(operation.method, operation.path)], then mutate relevant keys present in that payload.
🤖 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 `@tests/backend_api/test_openapi_contract.py` around lines 90 - 101, The test
in test_edge_case_payloads_are_accepted_by_contract_mock is using hard-coded
field names (email, password, mfa_code, client_fingerprint) that may not match
the selected operation's actual schema. Instead of creating a fixed payload
dictionary, retrieve the valid payload baseline for the selected operation using
valid_payloads[(operation.method, operation.path)], make a copy of it, and then
apply edge-case mutations only to the keys that exist in that baseline payload
(such as replacing values with unicode strings or extremely long strings). This
makes the test schema-aware and prevents failures due to missing or unexpected
fields.
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
1 participant
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.
Summary
Add order book depth updates, order cancellation, and snapshot immutability unit tests to cover standard and edge-case order book behaviors.
Changes
market/orderbook/orderbook_test.gowith Go unit tests verifying:ErrOrderNotFound.ErrBookClosed.tools/encryptly/linux-arm64/encryptlywith a Python wrapper script to support compilation in Termux.build.py's_normalize_os()helper.diagnostic/build-e84e063f.logdTesting
go test -v ./orderbookChecklist
/claim #2
Summary by CodeRabbit
Tests
New Features
Documentation
Chores