[MPT-14909] Added e2e tests for commerce order subscription #165
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdds a new e2e config key and introduces subscription-related pytest fixtures plus sync and async end-to-end tests for order subscription CRUD, listing, filtering, and teardown; includes a time-frozen factory producing deterministic subscription payloads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-12-12T15:02:20.732ZApplied to files:
📚 Learning: 2025-11-27T00:05:54.701ZApplied to files:
🧬 Code graph analysis (2)tests/e2e/commerce/order/subscription/conftest.py (1)
tests/e2e/commerce/order/subscription/test_async_subscription.py (6)
🪛 Ruff (0.14.8)tests/e2e/commerce/order/subscription/test_sync_subscription.py20-20: Unused Remove unused (RUF100) 42-42: Unused function argument: (ARG001) 53-53: Unused function argument: (ARG001) tests/e2e/commerce/order/subscription/test_async_subscription.py20-20: Unused Remove unused (RUF100) 44-44: Unused function argument: (ARG001) 55-55: Unused function argument: (ARG001) ⏰ 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)
🔇 Additional comments (5)
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
🧹 Nitpick comments (2)
tests/e2e/commerce/order/subscription/test_sync_subscription.py (1)
22-22: Remove unusednoqadirective.The
WPS421rule is not recognized by the current linter configuration. Remove the unused directive.- print(f"TEARDOWN - Unable to delete subscription: {getattr(error, 'title', str(error))}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete subscription: {getattr(error, 'title', str(error))}")tests/e2e/commerce/order/subscription/test_async_subscription.py (1)
22-22: Remove unusednoqadirective.Same as the sync test file - the
WPS421rule is not recognized by the current linter configuration.- print(f"TEARDOWN - Unable to delete subscription: {getattr(error, 'title', str(error))}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete subscription: {getattr(error, 'title', str(error))}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e_config.test.json(1 hunks)tests/e2e/commerce/conftest.py(2 hunks)tests/e2e/commerce/order/subscription/conftest.py(1 hunks)tests/e2e/commerce/order/subscription/test_async_subscription.py(1 hunks)tests/e2e/commerce/order/subscription/test_sync_subscription.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/commerce/conftest.pytests/e2e/commerce/order/subscription/test_async_subscription.pytests/e2e/commerce/order/subscription/conftest.pytests/e2e/commerce/order/subscription/test_sync_subscription.py
🧬 Code graph analysis (2)
tests/e2e/commerce/order/subscription/conftest.py (1)
tests/e2e/commerce/conftest.py (2)
agreement_subscription_line_id(61-62)factory(73-94)
tests/e2e/commerce/order/subscription/test_sync_subscription.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/commerce/order/subscription/conftest.py (1)
order_subscription_factory(6-24)tests/e2e/commerce/conftest.py (3)
order_id(16-17)subscription_id(56-57)invalid_subscription_id(66-67)
🪛 Ruff (0.14.8)
tests/e2e/commerce/order/subscription/test_async_subscription.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: created_order_subscription
(ARG001)
45-45: Unused function argument: created_order_subscription
(ARG001)
tests/e2e/commerce/order/subscription/conftest.py
11-11: Unused function argument: quantity
(ARG001)
tests/e2e/commerce/order/subscription/test_sync_subscription.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: created_order_subscription
(ARG001)
45-45: Unused function argument: created_order_subscription
(ARG001)
🔇 Additional comments (4)
e2e_config.test.json (1)
29-29: LGTM!The new configuration key aligns with the
agreement_subscription_line_idfixture and supports the order subscription test infrastructure.tests/e2e/commerce/conftest.py (1)
55-96: LGTM!The new fixtures follow the established patterns in this file. The
subscription_factorycorrectly uses all its parameters includingquantity.tests/e2e/commerce/order/subscription/test_sync_subscription.py (1)
34-41: Unusedcreated_order_subscriptionargument is intentional for test setup.The
created_order_subscriptionfixture is intentionally included to ensure a subscription exists before the test runs, even though the result isn't directly used. This is a valid pattern for fixtures with side effects. Consider adding a brief comment to clarify this intent.tests/e2e/commerce/order/subscription/test_async_subscription.py (1)
71-74: Good use of synchronous test function with async fixture.Correctly using
definstead ofasync defsince the test contains noawaitexpressions. pytest-asyncio will resolve the async fixture automatically. Based on learnings.
tests/e2e/commerce/order/subscription/test_async_subscription.py
Outdated
Show resolved
Hide resolved
tests/e2e/commerce/order/subscription/test_sync_subscription.py
Outdated
Show resolved
Hide resolved
robcsegal
force-pushed
the
MPT-14909-add-e-2-e-tests-for-commerce-order-subscription
branch
from
642ea3a to
2d0ae2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/commerce/conftest.py (1)
70-96: Code duplication with tests/e2e/commerce/subscription/conftest.py.The
subscription_factoryfixture is duplicated between this file andtests/e2e/commerce/subscription/conftest.py(lines 15-40). Both factories have identical logic, parameters, and structure.Consider consolidating these factories to eliminate duplication. Options:
- Keep only the factory in the more general location (
tests/e2e/commerce/conftest.py) and remove the duplicate in the subscription-specific conftest- Extract to a shared utility module if needed in multiple locations
Apply this pattern to maintain DRY:
# Keep in tests/e2e/commerce/conftest.py (this file) @pytest.fixture def subscription_factory(subscription_agreement_id, subscription_item_id): @freeze_time("2025-11-14T09:00:00.000Z") def factory( name: str = "E2E Created Subscription", external_vendor_id: str = "ext-vendor-id", quantity: int = 1, ): return { "name": name, "startDate": "2025-11-03T09:00:00.000Z", "commitmentDate": "2026-11-02T09:00:00.000Z", "autoRenew": True, "agreement": {"id": subscription_agreement_id}, "externalIds": {"vendor": external_vendor_id}, "template": None, "lines": [ { "item": {"id": subscription_item_id}, "quantity": quantity, "price": {"unitPP": 10}, } ], "parameters": {"fulfillment": []}, } return factoryAnd remove the duplicate from
tests/e2e/commerce/subscription/conftest.py.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e_config.test.json(1 hunks)tests/e2e/commerce/conftest.py(2 hunks)tests/e2e/commerce/order/subscription/conftest.py(1 hunks)tests/e2e/commerce/order/subscription/test_async_subscription.py(1 hunks)tests/e2e/commerce/order/subscription/test_sync_subscription.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/commerce/order/subscription/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/commerce/conftest.pytests/e2e/commerce/order/subscription/test_sync_subscription.pytests/e2e/commerce/order/subscription/test_async_subscription.py
🧬 Code graph analysis (2)
tests/e2e/commerce/conftest.py (5)
tests/e2e/commerce/subscription/conftest.py (1)
factory(18-39)tests/e2e/conftest.py (1)
e2e_config(89-92)tests/e2e/commerce/order/subscription/conftest.py (1)
factory(8-21)tests/e2e/commerce/asset/conftest.py (1)
factory(16-35)tests/e2e/commerce/agreement/conftest.py (1)
factory(20-49)
tests/e2e/commerce/order/subscription/test_async_subscription.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/commerce/order/subscription/conftest.py (1)
order_subscription_factory(6-23)tests/e2e/commerce/conftest.py (3)
order_id(16-17)subscription_id(56-57)invalid_subscription_id(66-67)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
tests/e2e/commerce/order/subscription/test_sync_subscription.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: created_order_subscription
(ARG001)
45-45: Unused function argument: created_order_subscription
(ARG001)
tests/e2e/commerce/order/subscription/test_async_subscription.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: created_order_subscription
(ARG001)
45-45: Unused function argument: created_order_subscription
(ARG001)
⏰ 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 (18)
e2e_config.test.json (1)
29-29: LGTM!The new configuration key is properly integrated with the new order subscription test fixtures and follows the existing naming convention.
tests/e2e/commerce/conftest.py (3)
2-2: LGTM!The
freezegunimport is correctly used in thesubscription_factoryfixture to ensure deterministic timestamps in test data.
55-62: LGTM!The new fixtures properly retrieve configuration values and follow the established pattern for e2e config fixtures.
65-67: LGTM!The invalid subscription ID fixture provides a well-formed but non-existent ID for negative test cases.
tests/e2e/commerce/order/subscription/test_async_subscription.py (7)
1-6: LGTM!The imports and test markers are appropriate for async e2e tests. The
flakymarker is a good practice for tests that interact with external APIs.
25-32: LGTM!The test correctly validates retrieval of an order subscription by ID.
34-42: LGTM!The test correctly validates listing order subscriptions. The
created_order_subscriptionfixture parameter is necessary to ensure at least one subscription exists before listing—this is a standard e2e test pattern. The static analysis warning about the unused parameter is a false positive.
44-51: LGTM!The test correctly validates the 404 error handling for non-existent subscriptions. While the
created_order_subscriptionfixture parameter appears unused, it may be included for consistency or to ensure the API is in a valid state before testing the negative case.
54-66: LGTM!The test correctly validates filtering order subscriptions using RQLQuery with multiple filters and field selection. The past review comment about removing an unused
subscription_idfixture parameter appears to have already been addressed.
69-85: LGTM!Both tests are correctly implemented:
test_create_order_subscriptionappropriately uses a synchronous function signature since it only verifies the fixture result (per project learnings)test_update_order_subscriptioncorrectly uses async/await for the API update call
88-93: LGTM!The test correctly validates the delete operation. No assertion is needed since the operation either succeeds or raises an exception.
tests/e2e/commerce/order/subscription/test_sync_subscription.py (7)
1-6: LGTM!The imports and test markers are appropriate for synchronous e2e tests.
25-32: LGTM!The test correctly validates retrieval of an order subscription by ID.
34-42: LGTM!The test correctly validates listing order subscriptions. The
created_order_subscriptionfixture parameter is necessary to ensure at least one subscription exists before listing. The static analysis warning is a false positive.
44-51: LGTM!The test correctly validates the 404 error handling for non-existent subscriptions. While the
created_order_subscriptionfixture parameter appears unused, it may be included for consistency or to ensure the API is in a valid state.
54-66: LGTM!The test correctly validates filtering order subscriptions using RQLQuery. The past review comment about removing an unused
subscription_idfixture parameter appears to have already been addressed.
69-85: LGTM!Both tests correctly validate creation and update operations for order subscriptions.
88-94: LGTM!The test correctly validates the delete operation. No assertion is needed since the operation either succeeds or raises an exception.
robcsegal
force-pushed
the
MPT-14909-add-e-2-e-tests-for-commerce-order-subscription
branch
3 times, most recently
from
17635e2 to
3119766
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/commerce/order/subscription/test_sync_subscription.py (1)
22-22: Remove unusednoqadirective.The
# noqa: WPS421directive is for wemake-python-styleguide. If the project uses only Ruff for linting, this directive is unnecessary.Apply this diff:
- print(f"TEARDOWN - Unable to delete subscription: {getattr(error, 'title', str(error))}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete subscription: {getattr(error, 'title', str(error))}")tests/e2e/commerce/order/subscription/test_async_subscription.py (1)
22-22: Remove unusednoqadirective.Same as the sync version - the
# noqa: WPS421directive is unnecessary if the project uses only Ruff for linting.Apply this diff:
- print(f"TEARDOWN - Unable to delete subscription: {getattr(error, 'title', str(error))}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete subscription: {getattr(error, 'title', str(error))}")
🧹 Nitpick comments (1)
tests/e2e/commerce/order/subscription/conftest.py (1)
7-21: Consider removing@freeze_timeif dates are hardcoded.The
@freeze_timedecorator freezes the current time for deterministic behavior, but the factory returns hardcoded date strings ("2025-11-03T09:00:00.000Z"and"2026-11-02T09:00:00.000Z") rather than computed dates. If these dates should remain static, the@freeze_timedecorator is unnecessary and can be removed for clarity.If the dates should be dynamically computed relative to "now" (e.g.,
datetime.now(),datetime.now() + timedelta(days=365)), then the decorator is appropriate, but the implementation should usedatetimecalls instead of hardcoded strings.Apply this diff if dates should remain static:
- @freeze_time("2025-11-14T09:00:00.000Z") def factory( name: str = "E2E Created Order Subscription", external_vendor_id: str = "ext-vendor-id",Or update to use dynamic dates if that's the intent:
+from datetime import datetime, timedelta + @pytest.fixture def order_subscription_factory(agreement_subscription_line_id): @freeze_time("2025-11-14T09:00:00.000Z") def factory( name: str = "E2E Created Order Subscription", external_vendor_id: str = "ext-vendor-id", ): + start_date = datetime.now().isoformat() + "Z" + commitment_date = (datetime.now() + timedelta(days=365)).isoformat() + "Z" return { "name": name, - "startDate": "2025-11-03T09:00:00.000Z", - "commitmentDate": "2026-11-02T09:00:00.000Z", + "startDate": start_date, + "commitmentDate": commitment_date, "autoRenew": True, "externalIds": {"vendor": external_vendor_id}, "template": None, "lines": [{"id": agreement_subscription_line_id}], "parameters": {"fulfillment": []}, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e_config.test.json(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/commerce/order/subscription/conftest.py(1 hunks)tests/e2e/commerce/order/subscription/test_async_subscription.py(1 hunks)tests/e2e/commerce/order/subscription/test_sync_subscription.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/commerce/conftest.py
- e2e_config.test.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/commerce/order/subscription/test_async_subscription.pytests/e2e/commerce/order/subscription/test_sync_subscription.pytests/e2e/commerce/order/subscription/conftest.py
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/e2e/commerce/order/subscription/test_async_subscription.py
🧬 Code graph analysis (3)
tests/e2e/commerce/order/subscription/test_async_subscription.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/commerce/order/subscription/conftest.py (1)
order_subscription_factory(6-23)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/commerce/order/subscription/test_sync_subscription.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/commerce/order/subscription/test_async_subscription.py (8)
created_order_subscription(10-22)test_get_order_subscription_by_id(25-31)test_list_order_subscriptions(34-41)test_get_order_subscription_by_id_not_found(44-51)test_filter_order_subscriptions(54-66)test_create_order_subscription(69-72)test_update_order_subscription(75-85)test_delete_order_subscription(88-93)tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/commerce/order/subscription/conftest.py (1)
order_subscription_factory(6-23)
tests/e2e/commerce/order/subscription/conftest.py (1)
tests/e2e/commerce/conftest.py (1)
agreement_subscription_line_id(60-61)
🪛 Ruff (0.14.8)
tests/e2e/commerce/order/subscription/test_async_subscription.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: created_order_subscription
(ARG001)
45-45: Unused function argument: created_order_subscription
(ARG001)
tests/e2e/commerce/order/subscription/test_sync_subscription.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: created_order_subscription
(ARG001)
45-45: Unused function argument: created_order_subscription
(ARG001)
⏰ 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 (1)
tests/e2e/commerce/order/subscription/test_async_subscription.py (1)
69-72: Correct use of synchronous test with async fixture.The test correctly uses
definstead ofasync defsince there's noawaitin the function body. Pytest-asyncio automatically resolves the async fixturecreated_order_subscriptionbefore passing it to the test. This follows the project's established pattern.Based on learnings, this pattern avoids unnecessary async functions when no await is needed.
tests/e2e/commerce/order/subscription/test_async_subscription.py
Outdated
Show resolved
Hide resolved
robcsegal
force-pushed
the
MPT-14909-add-e-2-e-tests-for-commerce-order-subscription
branch
from
3119766 to
a47325a
Compare
|
robcsegal
deleted the
MPT-14909-add-e-2-e-tests-for-commerce-order-subscription
branch
Added e2e tests for commerce order subscription
https://softwareone.atlassian.net/browse/MPT-14909
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.