[MPT-14908] Added seeding and e2e tests for commerce subscriptions #161
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdded Terminate and Update mixins and wired them into subscription services; added e2e config keys and fixtures; implemented new sync/async e2e subscription tests; introduced unit tests for terminate mixins; and updated pyproject lint/test markers and ignores. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 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)
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 |
✅ Found Jira issue key in the title: MPT-14908 Tests mirroring check (created files only)
|
![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
🧹 Nitpick comments (4)
mpt_api_client/resources/commerce/subscriptions.py (1)
55-57: Approve the signature change, but consider fixing the docstrings.The change to make
resource_dataoptional is backward compatible and allows terminate to be called with or without a payload. The implementation correctly handles the None case.However, while you're modifying these methods, consider fixing the pre-existing docstring issues:
- Lines 61-62 and 99-100 reference "Order" instead of "Subscription"
- Lines 65 and 103 incorrectly describe the return value as "template text in markdown format" when it actually returns a Subscription model
Apply this diff to correct the sync terminate docstring:
def terminate( self, resource_id: str, resource_data: ResourceData | None = None ) -> Subscription: """Terminate subscription. Args: - resource_id: Order resource ID - resource_data: Order resource data + resource_id: Subscription resource ID + resource_data: Subscription resource data Returns: - Subscription template text in markdown format. + Terminated subscription. """Apply this diff to correct the async terminate docstring:
async def terminate( self, resource_id: str, resource_data: ResourceData | None = None ) -> Subscription: """Terminate subscription. Args: - resource_id: Order resource ID - resource_data: Order resource data + resource_id: Subscription resource ID + resource_data: Subscription resource data Returns: - Subscription template text in markdown format. + Terminated subscription. """Also applies to: 93-95
tests/e2e/commerce/conftest.py (1)
14-21: Consider a clearer failure message when config keys are missing.Direct
e2e_config["..."]will raise a KeyError with a somewhat opaque message; for e2e onboarding it can be helpful to raise with a custom message (optional).tests/e2e/commerce/subscription/test_async_subscription.py (1)
42-53: Potential flake: filter asserts exactly one result.If the seeded environment can contain multiple matching subscriptions, consider asserting “>= 1” or tightening the filter predicate to a unique field to reduce flakes.
tests/e2e/commerce/subscription/test_sync_subscription.py (1)
42-53: Potential flake:len(result) == 1depends on seeded data uniqueness.Consider tightening the filter to a unique value (or relaxing the assertion) if environments can accumulate data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/subscriptions.py(5 hunks)pyproject.toml(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/commerce/subscription/conftest.py(1 hunks)tests/e2e/commerce/subscription/test_async_subscription.py(1 hunks)tests/e2e/commerce/subscription/test_sync_subscription.py(1 hunks)tests/unit/resources/commerce/test_subscriptions.py(1 hunks)
🧰 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/unit/resources/commerce/test_subscriptions.pytests/e2e/commerce/subscription/test_async_subscription.pytests/e2e/commerce/subscription/conftest.pytests/e2e/commerce/conftest.pytests/e2e/commerce/subscription/test_sync_subscription.py
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
pyproject.toml
🧬 Code graph analysis (5)
tests/unit/resources/commerce/test_subscriptions.py (16)
tests/unit/resources/catalog/test_price_list_items.py (1)
test_methods_present(40-43)tests/unit/resources/catalog/test_pricing_policy_attachments.py (1)
test_methods_present(44-47)tests/unit/resources/catalog/test_product_term_variants.py (1)
test_methods_present(47-50)tests/unit/resources/catalog/test_product_terms.py (1)
test_methods_present(42-45)tests/unit/resources/catalog/test_products_documents.py (1)
test_methods_present(37-40)tests/unit/resources/catalog/test_products_item_groups.py (1)
test_methods_present(34-37)tests/unit/resources/catalog/test_products_media.py (1)
test_methods_present(37-40)tests/unit/resources/catalog/test_products_parameter_groups.py (1)
test_methods_present(39-42)tests/unit/resources/catalog/test_products_parameters.py (1)
test_methods_present(34-37)tests/unit/resources/commerce/test_agreements_attachments.py (1)
test_methods_present(36-39)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_invoices.py (1)
test_methods_present(27-30)
tests/e2e/commerce/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)mpt_api_client/resources/commerce/subscriptions.py (4)
terminate(55-67)terminate(93-105)render(43-53)render(81-91)mpt_api_client/models/model.py (1)
id(119-121)
mpt_api_client/resources/commerce/subscriptions.py (1)
mpt_api_client/http/mixins.py (2)
AsyncUpdateMixin(286-300)UpdateMixin(41-55)
tests/e2e/commerce/subscription/conftest.py (2)
tests/e2e/conftest.py (1)
e2e_config(89-92)tests/e2e/commerce/conftest.py (2)
subscription_agreement_id(20-21)subscription_item_id(15-16)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
🪛 Ruff (0.14.8)
tests/e2e/commerce/subscription/test_async_subscription.py
20-20: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/commerce/subscription/test_sync_subscription.py
20-20: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
⏰ 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 (7)
pyproject.toml (1)
123-123: LGTM!The linter ignores are appropriate given the UpdateMixin additions to the subscriptions service, which increase the number of base classes and imports.
tests/unit/resources/commerce/test_subscriptions.py (1)
21-21: LGTM!The test coverage correctly reflects the new update capability from UpdateMixin and the existing iterate capability from CollectionMixin. Both sync and async services are appropriately tested.
Also applies to: 28-28
mpt_api_client/resources/commerce/subscriptions.py (2)
10-10: LGTM!The UpdateMixin and AsyncUpdateMixin imports are correct and necessary to enable update operations on subscription resources.
Also applies to: 15-15
34-34: LGTM!The UpdateMixin additions correctly enable update operations for both sync and async subscription services, following the established pattern used for other resource services in the codebase.
Also applies to: 72-72
e2e_config.test.json (1)
36-38: Sanity check: keep e2e IDs non-production and documented.Given this file lives in-repo, please ensure these identifiers are safe to publish and stable for CI (and ideally add a short README note describing where they come from / how to refresh).
tests/e2e/commerce/subscription/test_async_subscription.py (1)
55-59: Good:test_create_subscriptionis sync despite using an async fixture (avoids “async without await” lint noise). Based on learnings.tests/e2e/commerce/subscription/test_sync_subscription.py (1)
6-6: Verifyflakymarker is registered in pytest config (same concern as async module).
robcsegal
force-pushed
the
MPT-14908-add-e-2-e-tests-for-commerce-subscriptions
branch
from
bb07490 to
b1f36d0
Compare
robcsegal
force-pushed
the
MPT-14908-add-e-2-e-tests-for-commerce-subscriptions
branch
from
b1f36d0 to
6d678fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tests/e2e/commerce/subscription/test_async_subscription.py (1)
17-20: Teardown should not swallow unexpectedMPTAPIErrorexceptions.Right now any
MPTAPIErrorduring termination is printed and ignored, which can hide real teardown failures. Consider only suppressing a known/expected “already terminated / 404 Not Found” case and re‑raising other errors so the test run fails visibly.tests/e2e/commerce/subscription/test_sync_subscription.py (1)
17-20: Don’t hide unexpected teardown failures in sync tests either.Catching
MPTAPIErrorand just printing the message here also masks real teardown problems. Prefer only suppressing a known 404/“already terminated” case and letting other errors propagate so the test run fails.
🧹 Nitpick comments (1)
mpt_api_client/resources/commerce/subscriptions.py (1)
60-65: Docstrings still mention “Order” instead of “Subscription” (optional cleanup).The
terminatedocstrings refer to “Order resource ID/data” and “Subscription template text in markdown format”, which is inconsistent for a subscriptions service. Consider updating the wording to “Subscription resource ID/data” and clarify the actual return shape (subscription object vs markdown text).Also applies to: 98-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/subscriptions.py(5 hunks)pyproject.toml(2 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/commerce/subscription/conftest.py(1 hunks)tests/e2e/commerce/subscription/test_async_subscription.py(1 hunks)tests/e2e/commerce/subscription/test_sync_subscription.py(1 hunks)tests/unit/resources/commerce/test_subscriptions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/resources/commerce/test_subscriptions.py
- tests/e2e/commerce/conftest.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
pyproject.tomltests/e2e/commerce/subscription/test_sync_subscription.py
📚 Learning: 2025-12-12T18:15:22.101Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 161
File: tests/e2e/commerce/subscription/conftest.py:15-41
Timestamp: 2025-12-12T18:15:22.101Z
Learning: In tests for the marketplace API commerce subscriptions, do not assume externalIds.vendor (external_vendor_id) must be unique or enforce uniqueness constraints. Update tests to avoid asserting uniqueness for this field and document that the API does not enforce it. This guideline applies to tests under tests/e2e/commerce/subscription (and similar e2e tests) where external_vendor_id is used.
Applied to files:
tests/e2e/commerce/subscription/conftest.pytests/e2e/commerce/subscription/test_async_subscription.pytests/e2e/commerce/subscription/test_sync_subscription.py
📚 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/subscription/conftest.pytests/e2e/commerce/subscription/test_async_subscription.pytests/e2e/commerce/subscription/test_sync_subscription.py
🧬 Code graph analysis (2)
mpt_api_client/resources/commerce/subscriptions.py (1)
mpt_api_client/http/mixins.py (2)
AsyncUpdateMixin(286-300)UpdateMixin(41-55)
tests/e2e/commerce/subscription/test_async_subscription.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/commerce/subscription/conftest.py (2)
subscription_factory(16-41)subscription_id(6-7)mpt_api_client/resources/commerce/subscriptions.py (4)
terminate(55-67)terminate(93-105)render(43-53)render(81-91)mpt_api_client/models/model.py (1)
id(119-121)
🪛 GitHub Actions: PR build and merge
tests/e2e/commerce/subscription/test_async_subscription.py
[error] 1-1: wemake-python-styleguide WPS202 Found too many module members: 9 > 7.
tests/e2e/commerce/subscription/test_sync_subscription.py
[error] 1-1: wemake-python-styleguide WPS202 Found too many module members: 9 > 7.
🪛 Ruff (0.14.8)
tests/e2e/commerce/subscription/test_async_subscription.py
20-20: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/commerce/subscription/test_sync_subscription.py
20-20: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (6)
pyproject.toml (1)
77-79: Flaky marker registration and commerce per-file-ignores look consistent.Registering
flakyundermarkersand adding WPS ignores formpt_api_client/resources/commerce/*.pyaligns with existing patterns and resolves earlier unknown-marker/lint noise without affecting behavior.Also applies to: 126-126
tests/e2e/commerce/subscription/test_async_subscription.py (1)
23-82: Async subscription test coverage looks solid and matches service surface.The tests exercise get, list, 404 handling, filtered iteration, create, update, terminate, and render, using the async service API and shared fixtures correctly;
test_create_subscriptionremains sync while consuming an async fixture, which matches the pytest-asyncio pattern from prior work.e2e_config.test.json (1)
37-39: New subscription IDs in e2e config are consistent with existing conventions.The added
commerce.subscription.*keys match the established naming pattern and line up with how the new fixtures read frome2e_config.tests/e2e/commerce/subscription/conftest.py (1)
5-41: Subscription fixtures are clear and aligned with test needs.
subscription_id,invalid_subscription_id, and the time-frozensubscription_factoryprovide stable, reusable data for the e2e tests without enforcing unnecessary constraints onexternalIds.vendor, matching the API’s behavior.tests/e2e/commerce/subscription/test_sync_subscription.py (1)
23-82: Sync subscription e2e coverage matches async tests and new service capabilities.The sync suite exercises the same flows (get, list, 404, filter+iterate, create, update, terminate, render) using the sync client and shared fixtures, which gives good end-to-end coverage of the new subscription service surface.
mpt_api_client/resources/commerce/subscriptions.py (1)
32-38: Update mixins on sync/async subscription services look correct.Extending
SubscriptionsServicewithUpdateMixin[Subscription]andAsyncSubscriptionsServicewithAsyncUpdateMixin[Subscription]matches the shared mixin design and the newupdate-focused tests.Also applies to: 70-76
robcsegal
force-pushed
the
MPT-14908-add-e-2-e-tests-for-commerce-subscriptions
branch
from
6d678fc to
d0f78d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/commerce/subscription/test_async_subscription.py (1)
17-20: Teardown error handling refinement already discussed.The teardown currently catches all
MPTAPIErrorexceptions. Per past review feedback, consider distinguishing expected 404 (already terminated) from unexpected failures to avoid masking real issues.tests/e2e/commerce/subscription/test_sync_subscription.py (1)
17-20: Teardown error handling already discussed.Same pattern as async variant. Per past review feedback, consider refining to only suppress expected 404 errors.
🧹 Nitpick comments (5)
tests/e2e/commerce/subscription/test_async_subscription.py (1)
73-76: Consider fixture teardown interaction.
test_terminate_subscriptionterminates the subscription, then thecreated_subscriptionfixture teardown attempts termination again. The teardown's error handling accommodates this, but documenting this interaction (e.g., a brief comment) would improve clarity.mpt_api_client/resources/commerce/mixins.py (2)
7-17: Passingjson=Nonemay send a null body instead of no body.When
resource_dataisNone, calling_resource_action(..., json=resource_data)sends a JSON"null"payload. Some APIs distinguish between no body and a null body.Consider conditionally including the
jsonkwarg:def terminate(self, resource_id: str, resource_data: ResourceData | None = None) -> Model: - return self._resource_action(resource_id, "POST", "terminate", json=resource_data) # type: ignore[attr-defined, no-any-return] + kwargs: dict[str, ResourceData] = {} + if resource_data is not None: + kwargs["json"] = resource_data + return self._resource_action(resource_id, "POST", "terminate", **kwargs) # type: ignore[attr-defined, no-any-return]
23-33: Apply the same fix to the async variant.The same
json=Noneconcern applies here. Apply the conditional kwargs pattern for consistency.async def terminate(self, resource_id: str, resource_data: ResourceData | None = None) -> Model: - return await self._resource_action(resource_id, "POST", "terminate", json=resource_data) # type: ignore[attr-defined, no-any-return] + kwargs: dict[str, ResourceData] = {} + if resource_data is not None: + kwargs["json"] = resource_data + return await self._resource_action(resource_id, "POST", "terminate", **kwargs) # type: ignore[attr-defined, no-any-return]tests/unit/resources/commerce/test_mixins.py (2)
17-17: Consider simplifying the endpoint name.The endpoint name
"public/v1/dummy/terminate"results in URLs likepublic/v1/dummy/terminate/DUMMY-123/terminate, which includes "terminate" twice. While this works for testing, using"public/v1/dummy"as the endpoint would be clearer and avoid the redundancy.Apply this diff to simplify:
- _endpoint = "public/v1/dummy/terminate" + _endpoint = "public/v1/dummy"And update the corresponding mock URLs in tests (lines 42, 57, 72, 89).
Also applies to: 25-25
39-98: Strengthen test assertions and verify request data.All four tests only assert
result is not None, which provides minimal validation. The tests should also:
- Verify the returned model contains the expected data (id, status, name)
- When passing
resource_data, verify the request body was sent correctly using respx's request matchingThis will improve test reliability and catch more potential issues.
Example improvements for
test_terminate_with_data:def test_terminate_with_data(dummy_terminate_service): dummy_expected = {"id": "DUMMY-123", "status": "Terminated", "name": "Terminated DUMMY-123"} with respx.mock: - respx.post("https://api.example.com/public/v1/dummy/terminate/DUMMY-123/terminate").mock( + route = respx.post( + "https://api.example.com/public/v1/dummy/terminate/DUMMY-123/terminate", + json={"name": "Terminated DUMMY-123"} + ).mock( return_value=httpx.Response( status_code=httpx.codes.OK, json=dummy_expected, ) ) result = dummy_terminate_service.terminate("DUMMY-123", {"name": "Terminated DUMMY-123"}) assert result is not None + assert result.id == "DUMMY-123" + assert result.status == "Terminated" + assert result.name == "Terminated DUMMY-123" + assert route.calledApply similar changes to the other three tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/mixins.py(1 hunks)mpt_api_client/resources/commerce/subscriptions.py(3 hunks)pyproject.toml(2 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/commerce/subscription/conftest.py(1 hunks)tests/e2e/commerce/subscription/test_async_subscription.py(1 hunks)tests/e2e/commerce/subscription/test_sync_subscription.py(1 hunks)tests/unit/resources/commerce/test_mixins.py(1 hunks)tests/unit/resources/commerce/test_subscriptions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/commerce/subscription/conftest.py
- pyproject.toml
- e2e_config.test.json
- tests/e2e/commerce/conftest.py
🧰 Additional context used
🧠 Learnings (4)
📚 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/unit/resources/commerce/test_mixins.pytests/unit/resources/commerce/test_subscriptions.pytests/e2e/commerce/subscription/test_async_subscription.pytests/e2e/commerce/subscription/test_sync_subscription.py
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/commerce/subscription/test_async_subscription.pytests/e2e/commerce/subscription/test_sync_subscription.py
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/e2e/commerce/subscription/test_async_subscription.pytests/e2e/commerce/subscription/test_sync_subscription.py
📚 Learning: 2025-12-12T18:15:22.101Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 161
File: tests/e2e/commerce/subscription/conftest.py:15-41
Timestamp: 2025-12-12T18:15:22.101Z
Learning: In tests for the marketplace API commerce subscriptions, do not assume externalIds.vendor (external_vendor_id) must be unique or enforce uniqueness constraints. Update tests to avoid asserting uniqueness for this field and document that the API does not enforce it. This guideline applies to tests under tests/e2e/commerce/subscription (and similar e2e tests) where external_vendor_id is used.
Applied to files:
tests/e2e/commerce/subscription/test_async_subscription.pytests/e2e/commerce/subscription/test_sync_subscription.py
🧬 Code graph analysis (5)
tests/unit/resources/commerce/test_mixins.py (3)
mpt_api_client/http/async_service.py (1)
AsyncService(12-80)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/resources/commerce/mixins.py (4)
AsyncTerminateMixin(20-33)TerminateMixin(4-17)terminate(7-17)terminate(23-33)
mpt_api_client/resources/commerce/mixins.py (2)
mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/http/types.py (1)
json(40-42)
tests/unit/resources/commerce/test_subscriptions.py (16)
tests/unit/resources/catalog/test_price_list_items.py (1)
test_methods_present(40-43)tests/unit/resources/catalog/test_pricing_policy_attachments.py (1)
test_methods_present(44-47)tests/unit/resources/catalog/test_product_term_variants.py (1)
test_methods_present(47-50)tests/unit/resources/catalog/test_product_terms.py (1)
test_methods_present(42-45)tests/unit/resources/catalog/test_products_item_groups.py (1)
test_methods_present(34-37)tests/unit/resources/catalog/test_products_documents.py (1)
test_methods_present(37-40)tests/unit/resources/catalog/test_products_media.py (1)
test_methods_present(37-40)tests/unit/resources/catalog/test_products_parameter_groups.py (1)
test_methods_present(39-42)tests/unit/resources/catalog/test_products_parameters.py (1)
test_methods_present(34-37)tests/unit/resources/commerce/test_agreements_attachments.py (1)
test_methods_present(36-39)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)
mpt_api_client/resources/commerce/subscriptions.py (3)
mpt_api_client/http/mixins.py (2)
AsyncUpdateMixin(286-300)UpdateMixin(41-55)mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/resources/commerce/mixins.py (2)
AsyncTerminateMixin(20-33)TerminateMixin(4-17)
tests/e2e/commerce/subscription/test_async_subscription.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/commerce/subscription/conftest.py (2)
subscription_factory(16-41)subscription_id(6-7)mpt_api_client/resources/commerce/mixins.py (2)
terminate(7-17)terminate(23-33)mpt_api_client/resources/commerce/subscriptions.py (2)
render(42-52)render(66-76)
🪛 Ruff (0.14.8)
tests/e2e/commerce/subscription/test_async_subscription.py
20-20: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/commerce/subscription/test_sync_subscription.py
20-20: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
⏰ 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 (11)
tests/unit/resources/commerce/test_subscriptions.py (4)
21-32: LGTM!The expanded method coverage correctly validates that
update,iterate,terminate, andrenderare present on both sync and async subscription services, aligning with the new mixin additions.
35-51: LGTM!The async terminate test properly mocks the POST endpoint and validates the response model conversion.
54-67: LGTM!The render tests correctly validate the text response handling for the subscription template endpoint.
70-84: LGTM!The sync terminate test mirrors the async variant appropriately.
tests/e2e/commerce/subscription/test_async_subscription.py (2)
23-52: LGTM!The tests for get, list, 404 handling, and filtering are well-structured and follow consistent patterns.
55-58: LGTM!Correctly declared as synchronous
defsince it uses async fixtures but contains noawait. Based on learnings, pytest-asyncio resolves async fixtures automatically.tests/e2e/commerce/subscription/test_sync_subscription.py (1)
23-82: LGTM!The sync test implementations mirror the async variants appropriately, providing consistent e2e coverage for the synchronous API.
mpt_api_client/resources/commerce/subscriptions.py (3)
31-39: LGTM!Clean integration of
UpdateMixinandTerminateMixininto the class hierarchy. This centralizes the terminate logic and follows the established mixin pattern.
55-63: LGTM!Consistent async mixin integration matching the sync service pattern.
42-52: Therendermethod appropriately remains inline.Unlike
terminate, therendermethod returns raw text rather than a model instance, making it a reasonable candidate to keep as an inline method rather than extracting to a mixin.tests/unit/resources/commerce/test_mixins.py (1)
29-36: LGTM!Fixtures are properly structured and correctly instantiate the test service classes.
|
robcsegal
deleted the
MPT-14908-add-e-2-e-tests-for-commerce-subscriptions
branch
Added seeding and e2e tests for commerce subscriptions
https://softwareone.atlassian.net/browse/MPT-14908
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.