-
Notifications
You must be signed in to change notification settings - Fork 0
MPT-14896 E2E for catalog price list items #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe PR refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
c035f73 to
c17a870
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (1)
38-42: Consider renamingtest_create_price_list_item_invalid_datafor clarityThis test calls
updateon an existing item rather than performing a create. Renaming to something liketest_update_price_list_item_invalid_datawould better reflect the current service interface and avoid confusion now thatcreateisn’t part of the API.tests/e2e/catalog/price_lists/items/conftest.py (1)
4-26: Well-structured fixtures; consider guarding against empty pagesThe fixtures nicely centralize construction of sync/async services and reusable item data for the E2E suite. One minor consideration:
price_list_itemassumesfetch_page(1)always returns at least one item. If there’s any chance of an empty list in certain environments, adding a simple assertion or fallback before indexing (price_list_items[0]) would make failures easier to diagnose.tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py (1)
36-40: Align invalid-data test name with actual behaviour
test_create_price_list_item_invalid_dataexercises an update call on an existing item. Renaming the test to reference “update” instead of “create” would better reflect what’s happening and stay consistent with the service’s supported operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
mpt_api_client/resources/catalog/price_list_items.py(3 hunks)tests/e2e/catalog/price_lists/items/conftest.py(1 hunks)tests/e2e/catalog/price_lists/items/test_async_price_list_items.py(1 hunks)tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py(1 hunks)tests/unit/resources/catalog/test_price_list_items.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/helper.py (1)
assert_async_service_filter_with_iterate(31-39)tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py (5)
test_get_price_list_item(11-14)test_filter_price_list_items(17-20)test_update_price_list_item(23-26)test_get_price_list_item_not_found(29-33)test_create_price_list_item_invalid_data(36-40)tests/e2e/catalog/price_lists/items/conftest.py (3)
async_price_list_items_service(10-11)price_list_item(24-26)price_list_item_data(15-20)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/catalog/price_lists/items/conftest.py (2)
tests/e2e/conftest.py (3)
mpt_ops(31-32)async_mpt_ops(36-39)short_uuid(101-102)tests/e2e/catalog/price_lists/conftest.py (1)
price_list_id(29-30)
tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/helper.py (1)
assert_service_filter_with_iterate(42-50)tests/e2e/catalog/price_lists/items/conftest.py (3)
price_list_items_service(5-6)price_list_item(24-26)price_list_item_data(15-20)tests/unit/resources/catalog/test_price_list_items.py (1)
price_list_items_service(10-13)
tests/unit/resources/catalog/test_price_list_items.py (17)
tests/unit/resources/catalog/test_pricing_policy_attachments.py (1)
test_methods_present(44-47)tests/unit/resources/accounts/test_account_users.py (1)
test_methods_present(27-30)tests/unit/resources/accounts/test_accounts_users.py (1)
test_methods_present(43-46)tests/unit/resources/billing/test_credit_memo_attachments.py (1)
test_methods_present(42-45)tests/unit/resources/billing/test_custom_ledger_charges.py (1)
test_methods_present(40-43)tests/unit/resources/billing/test_custom_ledger_attachments.py (1)
test_methods_present(40-43)tests/unit/resources/billing/test_invoice_attachments.py (1)
test_methods_present(41-44)tests/unit/resources/billing/test_journal_attachments.py (1)
test_methods_present(41-44)tests/unit/resources/billing/test_journal_sellers.py (1)
test_methods_present(38-41)tests/unit/resources/billing/test_journal_upload.py (1)
test_methods_present(36-39)tests/unit/resources/billing/test_journal_charges.py (1)
test_methods_present(38-41)tests/unit/resources/billing/test_invoices.py (1)
test_methods_present(27-30)tests/unit/resources/billing/test_ledger_charges.py (1)
test_methods_present(36-39)tests/unit/resources/billing/test_ledger_attachments.py (1)
test_methods_present(41-44)tests/unit/resources/billing/test_statement_charges.py (1)
test_methods_present(40-43)tests/unit/resources/catalog/test_product_terms.py (1)
test_methods_present(42-45)tests/e2e/catalog/price_lists/items/conftest.py (1)
price_list_items_service(5-6)
mpt_api_client/resources/catalog/price_list_items.py (1)
mpt_api_client/http/mixins.py (4)
AsyncGetMixin(380-395)AsyncUpdateMixin(286-300)GetMixin(361-377)UpdateMixin(41-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
mpt_api_client/resources/catalog/price_list_items.py (1)
2-8: Mixins now correctly reflect read/update-only capabilitiesSwitching
PriceListItemsServiceandAsyncPriceListItemsServiceto useGetMixin/UpdateMixin(and their async counterparts) alongside the collection mixins cleanly expresses the supported operations and aligns with the new tests. MRO still looks correct for_resource_actionand config attributes; no issues spotted.Also applies to: 25-37
tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (1)
1-42: Good async coverage for get/filter/update and error casesAsync tests exercise the main behaviours (successful get/filter/update plus 404 and 400 error paths) using the shared helper and fixtures, which matches the new service surface (get/update only). Structure and assertions look consistent with the sync suite.
tests/unit/resources/catalog/test_price_list_items.py (1)
39-39: Method presence checks now aligned with service capabilitiesRestricting the parametrized methods to
["get", "update"]for both sync and async services correctly reflects the move away from managed (create/delete) operations and keeps the unit tests in sync with the new mixin-based design.Also applies to: 46-46
tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py (1)
1-40: Comprehensive sync E2E coverage matches the new service surfaceThese tests cover the key behaviours of the sync price list items service: happy-path get/filter/update plus 404 and 400 error paths, using the shared fixtures and helper. This lines up well with the refactor to get/update-only capabilities.
|



Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.