[MPT-16279] Added e2e tests for commerce assets #162
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughThis pull request adds comprehensive end-to-end testing infrastructure for the commerce assets API. Changes include configuration keys for asset identifiers, a return type change for the asset render method from 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)
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: 0
🧹 Nitpick comments (2)
tests/e2e/commerce/asset/test_sync_asset.py (1)
19-22: Remove or update the noqa directive.The
noqa: WPS421directive references a wemake-python-styleguide rule, but Ruff flags it as unknown. If the project uses Ruff's print detection, usenoqa: T201instead, or remove the directive entirely if print statements are acceptable in test teardown.Apply this diff:
- print(f"TEARDOWN - Unable to terminate asset: {getattr(error, 'title', str(error))}") # noqa: WPS421 + print(f"TEARDOWN - Unable to terminate asset: {getattr(error, 'title', str(error))}") # noqa: T201tests/e2e/commerce/asset/test_async_asset.py (1)
19-22: Remove or update the noqa directive.The
noqa: WPS421directive references a wemake-python-styleguide rule, but Ruff flags it as unknown. If the project uses Ruff's print detection, usenoqa: T201instead, or remove the directive entirely if print statements are acceptable in test teardown.Apply this diff:
- print(f"TEARDOWN - Unable to terminate asset: {getattr(error, 'title', str(error))}") # noqa: WPS421 + print(f"TEARDOWN - Unable to terminate asset: {getattr(error, 'title', str(error))}") # noqa: T201
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/assets.py(3 hunks)tests/e2e/commerce/asset/conftest.py(1 hunks)tests/e2e/commerce/asset/test_async_asset.py(1 hunks)tests/e2e/commerce/asset/test_sync_asset.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/conftest.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/conftest.pytests/e2e/commerce/asset/conftest.pytests/e2e/commerce/asset/test_async_asset.pytests/e2e/commerce/asset/test_sync_asset.py
🧬 Code graph analysis (4)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/commerce/asset/conftest.py (2)
tests/e2e/conftest.py (1)
e2e_config(89-92)tests/e2e/commerce/conftest.py (2)
asset_item_id(15-16)asset_agreement_id(20-21)
mpt_api_client/resources/commerce/assets.py (1)
mpt_api_client/http/types.py (1)
text(36-38)
tests/e2e/commerce/asset/test_sync_asset.py (7)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/commerce/asset/test_async_asset.py (5)
created_asset(10-22)test_get_asset_by_id(25-28)test_list_assets(31-36)test_get_asset_by_id_not_found(39-41)test_filter_assets(44-54)tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/commerce/asset/conftest.py (3)
asset_factory(15-37)agreement_asset_id(5-6)invalid_agreement_asset_id(10-11)mpt_api_client/resources/commerce/assets.py (4)
terminate(42-50)terminate(76-87)render(52-63)render(89-100)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
tests/e2e/commerce/asset/test_async_asset.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/commerce/asset/test_sync_asset.py
22-22: 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)
tests/e2e/commerce/conftest.py (1)
14-21: LGTM!The new fixtures follow the established pattern and cleanly expose asset-specific IDs from the E2E configuration.
tests/e2e/commerce/asset/test_sync_asset.py (1)
25-83: LGTM!The test suite provides comprehensive coverage of asset operations including CRUD, filtering, and rendering. The test structure is clean and follows pytest conventions.
mpt_api_client/resources/commerce/assets.py (1)
52-63: LGTM - Breaking change is consistent.Both sync and async
rendermethods now return a string instead of anAssetTemplateobject. This is a breaking API change, but it's implemented consistently across both methods and the E2E tests validate the new behavior.Also applies to: 89-100
tests/e2e/commerce/asset/conftest.py (1)
4-37: LGTM!The fixtures are well-structured and follow established patterns. The
asset_factoryprovides a flexible way to create asset test data with sensible defaults.tests/e2e/commerce/asset/test_async_asset.py (2)
57-60: LGTM - Correct usage of sync test with async fixture.This test correctly uses
definstead ofasync defbecause it uses an async fixture but contains no await. Pytest-asyncio will resolve the async fixture automatically, as per the project pattern.
25-83: LGTM!The async test suite mirrors the sync tests with proper async/await usage and provides comprehensive E2E coverage of asset operations.
e2e_config.test.json (1)
29-33: No action needed on empty template ID.The
commerce.assets.product.template.idkey is not referenced in the tests, so the empty string value does not affect test execution.
robcsegal
force-pushed
the
MPT-16279-add-e-2-e-tests-for-commerce-assets
branch
from
9f27364 to
5211e77
Compare
robcsegal
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpt_api_client/resources/commerce/assets.py (1)
52-63: Fix sync docstring for consistency with async versionThe sync
renderdocstring says "Render asset template json." but should say "Render asset template string." to match the async version and accurately reflect that the method returns raw text content.def render(self, asset_id: str) -> str: """Renders the template for the given Asset id. Args: asset_id: Asset ID. Returns: - Render asset template json. + Rendered asset template string.
🧹 Nitpick comments (1)
tests/e2e/commerce/asset/test_async_asset.py (1)
57-61: Good use of sync test with async fixture (matches agreed pattern)
test_create_assetis a synchronousdefthat relies on the asynccreated_assetfixture without doing its ownawait, which aligns with the preferred pytest‑asyncio pattern for avoiding unnecessaryasync deftests. This is consistent with prior decisions for this repo.Based on learnings, this is the intended style for such tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/assets.py(3 hunks)tests/e2e/commerce/asset/conftest.py(1 hunks)tests/e2e/commerce/asset/test_async_asset.py(1 hunks)tests/e2e/commerce/asset/test_sync_asset.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e_config.test.json
- tests/e2e/commerce/asset/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📚 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/conftest.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/conftest.pytests/e2e/commerce/asset/test_async_asset.pytests/e2e/commerce/asset/test_sync_asset.py
🧬 Code graph analysis (4)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/commerce/asset/test_async_asset.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/commerce/asset/conftest.py (3)
asset_factory(15-37)agreement_asset_id(5-6)invalid_agreement_asset_id(10-11)mpt_api_client/resources/commerce/assets.py (4)
terminate(42-50)terminate(76-87)render(52-63)render(89-100)mpt_api_client/models/model.py (1)
id(119-121)
mpt_api_client/resources/commerce/assets.py (1)
mpt_api_client/http/types.py (1)
text(36-38)
tests/e2e/commerce/asset/test_sync_asset.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/commerce/asset/conftest.py (3)
asset_factory(15-37)agreement_asset_id(5-6)invalid_agreement_asset_id(10-11)mpt_api_client/resources/commerce/assets.py (4)
terminate(42-50)terminate(76-87)render(52-63)render(89-100)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
tests/e2e/commerce/asset/test_async_asset.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/commerce/asset/test_sync_asset.py
22-22: 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 (2)
tests/e2e/commerce/conftest.py (1)
44-51: Asset ID fixtures are consistent with existing e2e config usage
asset_item_idandasset_agreement_idfollow the same pattern as other commerce fixtures and correctly map to thecommerce.assets.*keys. No changes needed.tests/e2e/commerce/asset/test_sync_asset.py (1)
25-83: Sync asset E2E flows look comprehensive and robustThe tests exercise core asset flows (get, list, not-found, filter, create, update, terminate, render) and reuse the
created_assetfixture cleanly; double-termination is safely handled by the teardown’sMPTAPIErrorcatch. This gives good end‑to‑end coverage for the sync asset API.
|
Added e2e tests for commerce assets
https://softwareone.atlassian.net/browse/MPT-16279
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.