SQS-347 | integration, synthetic monitoring tests for active orders #504
Conversation
query Implements integration, synthetic monitoring tests for the for active orders query. Additionally adds test file for locust.
WalkthroughThis pull request introduces enhancements across multiple files, including the addition of the Changes
Poem
Tip Announcements
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (1)
Additional context usedRuff
Additional comments not posted (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range, codebase verification and nitpick comments (1)
locust/locustfile.py (1)
9-9: Remove unused importPassthroughOrderbookActiveOrders.The import
PassthroughOrderbookActiveOrdersinlocust/locustfile.pyis not used anywhere in the file. It is recommended to remove this unused import to maintain clean and efficient code.
- File:
locust/locustfile.py
- Line:
from passthrough_orderbook_active_orders import PassthroughOrderbookActiveOrdersAnalysis chain
Verify the usage of the new import.
It's important to ensure that the import
PassthroughOrderbookActiveOrdersis actually used in the file or planned for future use. If it's not used, it should be removed to avoid confusion and maintain clean code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `PassthroughOrderbookActiveOrders` in `locustfile.py`. # Test: Search for the usage of `PassthroughOrderbookActiveOrders`. Expect: At least one usage. rg --type python 'PassthroughOrderbookActiveOrders' locust/locustfile.pyLength of output: 103
Script:
#!/bin/bash # Description: Verify the usage of `PassthroughOrderbookActiveOrders` in `locustfile.py`. # Test: Search for the usage of `PassthroughOrderbookActiveOrders`. Expect: At least one usage. rg 'PassthroughOrderbookActiveOrders' locust/locustfile.pyLength of output: 139
Tools
Ruff
9-9:
passthrough_orderbook_active_orders.PassthroughOrderbookActiveOrdersimported but unusedRemove unused import:
passthrough_orderbook_active_orders.PassthroughOrderbookActiveOrders(F401)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- locust/locustfile.py (1 hunks)
- locust/passthrough_orderbook_active_orders.py (1 hunks)
- tests/active_orderbook_orders_response.py (1 hunks)
- tests/sqs_service.py (2 hunks)
- tests/test_passthrough.py (3 hunks)
- tests/test_synthetic_geo.py (1 hunks)
Additional context used
Ruff
locust/locustfile.py
9-9:
passthrough_orderbook_active_orders.PassthroughOrderbookActiveOrdersimported but unusedRemove unused import:
passthrough_orderbook_active_orders.PassthroughOrderbookActiveOrders(F401)
tests/test_passthrough.py
2-2:
conftestimported but unusedRemove unused import:
conftest(F401)
3-3:
pytestimported but unusedRemove unused import:
pytest(F401)
5-5:
from sqs_service import *used; unable to detect undefined names(F403)
8-8:
from e2e_math import *used; unable to detect undefined names(F403)
9-9:
from decimal import *used; unable to detect undefined names(F403)
51-51: Yoda condition detected
Rewrite as
elapsed_time_ms < EXPECTED_LATENCY_UPPER_BOUND_MS(SIM300)
51-51:
expected_latency_upper_bound_msmay be undefined, or defined from star imports(F405)
tests/test_synthetic_geo.py
46-46: Redefinition of unused
test_synth_passthrough_portfolio_assetsfrom line 41(F811)
Additional comments not posted (2)
locust/passthrough_orderbook_active_orders.py (1)
7-10: Basic functionality approved.The implementation of the
passthroughOrderbookActiveOrderstask method aligns with the PR objectives to enhance testing for active orders. The method correctly uses theHttpUserclient to make a GET request.tests/test_passthrough.py (1)
33-56: New test method for active orders approved.The new test method
test_active_orderbook_ordersand the helper functionrun_test_active_orderbook_ordersare well-implemented and align with the PR objectives to enhance testing for active orders. The method correctly measures latency and validates the response, which is crucial for performance testing.Tools
Ruff
51-51: Yoda condition detected
Rewrite as
elapsed_time_ms < EXPECTED_LATENCY_UPPER_BOUND_MS(SIM300)
51-51:
expected_latency_upper_bound_msmay be undefined, or defined from star imports(F405)
There was a problem hiding this comment.
LGTM overall but experienced this when running
================================================================ short test summary info =================================================================
FAILED tests/test_passthrough.py::TestPassthrough::test_active_orderbook_orders[http://localhost:9092] - NameError: name 'expected_latency_upper_bound_ms' is not defined
Let's please fix pre-merge
…uery Implement requested changes
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
tests/test_passthrough.py (1)
51-51: Consider rewriting the assertion condition for better readability.The assertion condition is using a Yoda condition, which is a style where the constant is placed on the left side of the comparison operator. While it is a valid condition, it can be rewritten in the conventional way for better readability.
Apply this diff to rewrite the assertion condition:
-assert EXPECTED_LATENCY_UPPER_BOUND_MS > elapsed_time_ms, f"Error: latency {elapsed_time_ms} exceeded {EXPECTED_LATENCY_UPPER_BOUND_MS} ms" +assert elapsed_time_ms < EXPECTED_LATENCY_UPPER_BOUND_MS, f"Error: latency {elapsed_time_ms} exceeded {EXPECTED_LATENCY_UPPER_BOUND_MS} ms"Tools
Ruff
51-51: Yoda condition detected
Rewrite as
elapsed_time_ms < EXPECTED_LATENCY_UPPER_BOUND_MS(SIM300)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- locust/locustfile.py (1 hunks)
- locust/passthrough_orderbook_active_orders.py (1 hunks)
- tests/test_passthrough.py (3 hunks)
- tests/test_synthetic_geo.py (2 hunks)
Files skipped from review due to trivial changes (1)
- locust/passthrough_orderbook_active_orders.py
Files skipped from review as they are similar to previous changes (1)
- tests/test_synthetic_geo.py
Additional context used
Ruff
locust/locustfile.py
8-8:
passthrough_orderbook_active_orders.PassthroughOrderbookActiveOrdersimported but unusedRemove unused import:
passthrough_orderbook_active_orders.PassthroughOrderbookActiveOrders(F401)
tests/test_passthrough.py
2-2:
conftestimported but unusedRemove unused import:
conftest(F401)
3-3:
pytestimported but unusedRemove unused import:
pytest(F401)
5-5:
from sqs_service import *used; unable to detect undefined names(F403)
8-8:
from e2e_math import *used; unable to detect undefined names(F403)
9-9:
from decimal import *used; unable to detect undefined names(F403)
51-51: Yoda condition detected
Rewrite as
elapsed_time_ms < EXPECTED_LATENCY_UPPER_BOUND_MS(SIM300)
Additional comments not posted (4)
tests/test_passthrough.py (4)
1-1: Also applies to: 2-2, 3-3, 5-5, 6-6, 8-8, 9-9
33-34: LGTM!The code changes are approved.
37-56: LGTM!The code changes are approved.
Tools
Ruff
51-51: Yoda condition detected
Rewrite as
elapsed_time_ms < EXPECTED_LATENCY_UPPER_BOUND_MS(SIM300)
55-55: LGTM!The code changes are approved.
|
All suggestions were addressed, merging. 🚀 |
…504) (#507) * SQS-347 | integration, synthetic monitoring tests for active orders query Implements integration, synthetic monitoring tests for the for active orders query. Additionally adds test file for locust. (cherry picked from commit 9de1489) Co-authored-by: Deividas Petraitis <[email protected]>
Implements integration, synthetic monitoring tests for the for active orders query. Additionally adds test file for locust.
Summary by CodeRabbit
New Features
Tests
/passthrough/active-ordersendpoint to ensure comprehensive testing.Chores