[MPT-16318] Added e2e tests for commerce order assets #166
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdds three commerce asset ID keys to test config, relocates two asset fixtures to a higher-scope conftest, introduces new order-asset ID fixtures and an order_asset_factory, and adds synchronous and asynchronous end-to-end tests for order-asset CRUD, list/filter, render, and teardown flows. 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: 1
🧹 Nitpick comments (8)
tests/e2e/commerce/order/asset/test_async_asset.py (2)
36-45: Consider usingpytest.mark.usefixturesfor implicit fixture dependencies.The
created_order_assetparameter is unused in the function body but required to ensure an asset exists. Using@pytest.mark.usefixtures("created_order_asset")makes this intent explicit and avoids the static analysis warning.+@pytest.mark.usefixtures("created_order_asset") async def test_list_order_assets( - async_mpt_vendor, created_order_asset, commerce_asset_draft_order_id + async_mpt_vendor, commerce_asset_draft_order_id ):
48-55: Same suggestion: usepytest.mark.usefixturesfor clarity.This test also uses
created_order_assetonly for its side effect (ensuring an asset exists). The decorator approach makes the intent clearer.+@pytest.mark.usefixtures("created_order_asset") async def test_get_order_asset_by_id_not_found( - async_mpt_vendor, created_order_asset, commerce_asset_draft_order_id, invalid_agreement_asset_id + async_mpt_vendor, commerce_asset_draft_order_id, invalid_agreement_asset_id ):tests/e2e/commerce/order/asset/test_sync_asset.py (6)
25-32: Consider strengthening the assertion.The test only checks that the result is not None. For more robust E2E testing, consider validating additional properties such as verifying the returned asset ID matches the requested one.
Example:
def test_get_order_asset_by_id(mpt_vendor, created_order_asset, commerce_asset_draft_order_id): asset_id = created_order_asset.id orders = mpt_vendor.commerce.orders.assets(commerce_asset_draft_order_id) result = orders.get(asset_id) assert result is not None assert result.id == asset_id
34-42: Consider verifying the created asset appears in results.While
created_order_assetis intentionally included for its side effect (as noted in the fixture comment), the test doesn't verify that the created asset actually appears in the results. This would make the dependency more explicit and strengthen the test.Example:
def test_list_order_assets(mpt_vendor, created_order_asset, commerce_asset_draft_order_id): limit = 10 orders = mpt_vendor.commerce.orders assets = orders.assets(commerce_asset_draft_order_id) result = assets.fetch_page(limit=limit) assert result is not None asset_ids = [asset.id for asset in result.items] assert created_order_asset.id in asset_ids
69-73: Consider validating created asset properties.The test only checks that creation succeeded. Consider asserting on specific properties like the asset name or other expected fields to make the test more meaningful.
Example:
def test_create_order_asset(created_order_asset): result = created_order_asset assert result is not None assert result.id is not None assert result.name == "E2E Created Order Asset"
75-90: Consider verifying the update was applied.The test should confirm that the name was actually updated to the expected value.
Example:
def test_update_order_asset( mpt_vendor, created_order_asset, commerce_asset_draft_order_id, ): asset_id = created_order_asset.id updated_asset_data = { "name": "E2E Updated Order Asset", } orders = mpt_vendor.commerce.orders assets = orders.assets(commerce_asset_draft_order_id) result = assets.update(asset_id, updated_asset_data) assert result is not None assert result.name == "E2E Updated Order Asset"
92-103: Consider verifying deletion succeeded.The test doesn't assert that the deletion was successful. Consider verifying that the asset no longer exists by attempting to retrieve it and expecting a 404 error.
Example:
def test_delete_order_asset( mpt_vendor, created_order_asset, commerce_asset_draft_order_id, ): asset_id = created_order_asset.id orders = mpt_vendor.commerce.orders assets = orders.assets(commerce_asset_draft_order_id) assets.delete(asset_id) # Verify deletion with pytest.raises(MPTAPIError, match="404 Not Found"): assets.get(asset_id)
105-116: Consider validating the rendered output.The test only checks that rendering returns a result. Consider asserting on properties of the rendered content to ensure it contains expected data.
Example (adjust based on expected render output):
def test_render_order_asset( mpt_vendor, created_order_asset, commerce_asset_draft_order_id, ): asset_id = created_order_asset.id orders = mpt_vendor.commerce.orders assets = orders.assets(commerce_asset_draft_order_id) result = assets.render(asset_id) assert result is not None assert len(result) > 0 # or other appropriate validation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e_config.test.json(1 hunks)pyproject.toml(1 hunks)tests/e2e/commerce/asset/conftest.py(0 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/commerce/order/asset/conftest.py(1 hunks)tests/e2e/commerce/order/asset/test_async_asset.py(1 hunks)tests/e2e/commerce/order/asset/test_sync_asset.py(1 hunks)
💤 Files with no reviewable changes (1)
- 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/order/asset/test_async_asset.pytests/e2e/commerce/order/asset/test_sync_asset.pytests/e2e/commerce/order/asset/conftest.py
🧬 Code graph analysis (2)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/commerce/order/asset/conftest.py (3)
tests/e2e/conftest.py (3)
e2e_config(89-92)buyer_id(132-133)buyer_account_id(137-138)tests/e2e/commerce/conftest.py (1)
commerce_product_id(10-11)tests/e2e/commerce/asset/conftest.py (1)
factory(6-25)
🪛 Ruff (0.14.8)
tests/e2e/commerce/order/asset/test_async_asset.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
37-37: Unused function argument: created_order_asset
(ARG001)
49-49: Unused function argument: created_order_asset
(ARG001)
tests/e2e/commerce/order/asset/test_sync_asset.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: created_order_asset
(ARG001)
45-45: Unused function argument: created_order_asset
(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 (9)
pyproject.toml (1)
135-135: LGTM!The lint suppressions are appropriate for the new e2e test directory and consistent with existing patterns for other test directories.
tests/e2e/commerce/conftest.py (1)
54-61: LGTM!The new fixtures follow the established pattern and provide necessary test data for order asset e2e tests.
e2e_config.test.json (1)
30-34: LGTM!The new configuration entries follow the established naming conventions and ID patterns.
tests/e2e/commerce/order/asset/conftest.py (1)
1-57: LGTM!The fixtures are well-structured and follow the established factory pattern used in other e2e test modules. The
order_asset_factoryprovides a clean interface for creating test payloads with sensible defaults.tests/e2e/commerce/order/asset/test_async_asset.py (2)
75-78: LGTM!Correctly declared as a synchronous function since it contains no
await. Pytest-asyncio will resolve the asynccreated_order_assetfixture automatically. Based on learnings, this is the preferred pattern.
98-108: Verify: Double-delete handled gracefully in teardown.This test explicitly deletes the asset, but the
created_order_assetfixture will also attempt deletion in its teardown. The fixture'stry/exceptblock handles this correctly, so no issue—just noting the intentional pattern.tests/e2e/commerce/order/asset/test_sync_asset.py (3)
1-6: LGTM!The imports and flaky marker are appropriate for end-to-end testing.
44-52: LGTM!The error handling test correctly verifies 404 responses. The
created_order_assetfixture is included for its side effect as documented in the fixture comment.
54-67: LGTM!The filtering test is well-structured with appropriate assertions. It correctly tests RQL query filtering and field selection.
robcsegal
force-pushed
the
MPT-16318-add-e-2-e-tests-for-commerce-order-assets
branch
from
5f3db99 to
eb3cf0d
Compare
robcsegal
force-pushed
the
MPT-16318-add-e-2-e-tests-for-commerce-order-assets
branch
from
eb3cf0d to
b42f485
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/commerce/order/asset/test_sync_asset.py (1)
11-20: Update the noqa directive for Ruff compatibility.The
WPS421code is from wemake-python-styleguide, but Ruff doesn't recognize it. Since this project uses both linters (per pyproject.toml), you can use# noqa: T201, WPS421to suppress print statement warnings in both.Apply this diff:
- print(f"TEARDOWN - Unable to delete order asset: {getattr(error, 'title', str(error))}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete order asset: {getattr(error, 'title', str(error))}") # noqa: T201, WPS421Based on learnings, the repository uses wemake-python-styleguide, so the WPS421 code should be retained alongside Ruff's T201.
🧹 Nitpick comments (1)
tests/e2e/commerce/order/asset/test_async_asset.py (1)
11-21: Update the noqa directive for Ruff compatibility.The
WPS421code is from wemake-python-styleguide, but Ruff doesn't recognize it. Since this project uses both linters (per pyproject.toml), you can use# noqa: T201, WPS421to suppress print statement warnings in both.Apply this diff:
- print(f"TEARDOWN - Unable to delete order asset: {getattr(error, 'title', str(error))}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete order asset: {getattr(error, 'title', str(error))}") # noqa: T201, WPS421Based on learnings, the repository uses wemake-python-styleguide, so the WPS421 code should be retained alongside Ruff's T201.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e_config.test.json(1 hunks)pyproject.toml(1 hunks)tests/e2e/commerce/asset/conftest.py(0 hunks)tests/e2e/commerce/asset/test_async_asset.py(2 hunks)tests/e2e/commerce/asset/test_sync_asset.py(2 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/commerce/order/asset/conftest.py(1 hunks)tests/e2e/commerce/order/asset/test_async_asset.py(1 hunks)tests/e2e/commerce/order/asset/test_sync_asset.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/commerce/asset/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/commerce/conftest.py
- tests/e2e/commerce/order/asset/conftest.py
🧰 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/asset/test_async_asset.pytests/e2e/commerce/order/asset/test_async_asset.pytests/e2e/commerce/order/asset/test_sync_asset.pytests/e2e/commerce/asset/test_sync_asset.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/order/asset/test_sync_asset.py
🧬 Code graph analysis (4)
tests/e2e/commerce/asset/test_async_asset.py (2)
tests/e2e/commerce/conftest.py (2)
asset_id(70-71)invalid_asset_id(75-76)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/commerce/order/asset/test_async_asset.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/commerce/order/asset/conftest.py (2)
order_asset_factory(20-57)commerce_asset_draft_order_id(15-16)tests/e2e/commerce/conftest.py (2)
asset_id(70-71)invalid_asset_id(75-76)
tests/e2e/commerce/order/asset/test_sync_asset.py (3)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/commerce/conftest.py (2)
asset_id(70-71)invalid_asset_id(75-76)
tests/e2e/commerce/asset/test_sync_asset.py (3)
tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/commerce/conftest.py (2)
asset_id(70-71)invalid_asset_id(75-76)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
tests/e2e/commerce/order/asset/test_async_asset.py
20-20: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
45-45: Unused function argument: created_order_asset
(ARG001)
57-57: Unused function argument: created_order_asset
(ARG001)
tests/e2e/commerce/order/asset/test_sync_asset.py
20-20: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
42-42: Unused function argument: created_order_asset
(ARG001)
53-53: Unused function argument: created_order_asset
(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 (11)
pyproject.toml (1)
135-135: LGTM!The per-file-ignores entry for the new order asset tests is consistent with existing patterns and appropriately suppresses complexity warnings that are expected in e2e test files.
tests/e2e/commerce/asset/test_sync_asset.py (1)
25-26: LGTM!The fixture parameter renames from
agreement_asset_idtoasset_idimprove clarity and align with the fixture definitions intests/e2e/commerce/conftest.py.Also applies to: 39-41, 44-47
tests/e2e/commerce/asset/test_async_asset.py (1)
25-26: LGTM!The fixture parameter renames from
agreement_asset_idtoasset_idimprove clarity and consistency across the async tests.Also applies to: 39-41, 44-47
e2e_config.test.json (1)
31-31: LGTM!The new configuration keys follow existing naming conventions and provide the necessary test data for the order asset e2e tests.
Also applies to: 34-35
tests/e2e/commerce/order/asset/test_async_asset.py (4)
23-30: LGTM!The
created_order_assetfixture properly uses the async context manager for resource lifecycle management, and the comment clarifies why this fixture is required for all tests.
44-53: Static analysis false positive: fixture is required for setup.The
created_order_assetfixture is intentionally included to ensure proper test setup state, as noted in the comment on line 25. This is a common pattern in e2e tests where fixtures establish necessary preconditions even if not directly referenced in the test body.Also applies to: 56-63
83-86: LGTM!Correctly uses
definstead ofasync deffor a test that uses async fixtures but contains no await statements. This follows the learned pattern for pytest-asyncio.Based on learnings, this pattern avoids linter complaints about unnecessary async functions.
33-130: Excellent test coverage for order asset operations.The test suite comprehensively covers CRUD operations (create, read, update, delete) plus additional operations (list, filter, render) with proper async patterns and error handling.
tests/e2e/commerce/order/asset/test_sync_asset.py (3)
23-30: LGTM!The
created_order_assetfixture properly uses the sync context manager for resource lifecycle management, mirroring the async implementation.
42-49: Static analysis false positive: fixture is required for setup.The
created_order_assetfixture is intentionally included to ensure proper test setup state, as noted in the comment on line 25. This matches the pattern used in the async test file.Also applies to: 52-59
33-124: Excellent test coverage for order asset operations.The synchronous test suite provides comprehensive coverage that mirrors the async implementation, ensuring both API interfaces are thoroughly tested.
|
robcsegal
deleted the
MPT-16318-add-e-2-e-tests-for-commerce-order-assets
branch
Added e2e tests for commerce order assets
https://softwareone.atlassian.net/browse/MPT-16318
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.