MPT-14900 E2E tests for catalog pricing-policies attachments #133
Conversation
WalkthroughRefactors pricing policy attachments to use CreateFile/DownloadFile mixins with explicit upload keys; adds sync and async end-to-end attachment tests and fixtures; removes activate/deactivate expectations from unit tests; centralizes pricing policy creation in fixtures; and removes Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant AttachmentService
participant CreateFileMixin
participant DownloadFileMixin
Note over Test,AttachmentService: Attachment lifecycle after mixin refactor
rect rgb(230,245,240)
Test->>AttachmentService: create(data, file)
AttachmentService->>CreateFileMixin: upload(file, data, file_key="file", data_key="attachment")
CreateFileMixin-->>AttachmentService: created Attachment
AttachmentService-->>Test: Attachment resource
end
rect rgb(245,245,250)
Test->>AttachmentService: get(attachment_id)
AttachmentService-->>Test: Attachment
end
rect rgb(245,235,255)
Test->>AttachmentService: download(attachment_id)
AttachmentService->>DownloadFileMixin: download(file_id)
DownloadFileMixin-->>AttachmentService: file content + filename
AttachmentService-->>Test: file bytes, filename
end
rect rgb(255,245,230)
Test->>AttachmentService: delete(attachment_id)
AttachmentService-->>Test: deletion result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
albertsola
force-pushed
the
e2e/catalog/MPT-14900-pricing-policy-attachments
branch
from
0e1014e to
4d263d8
Compare
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
![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 (5)
tests/e2e/catalog/pricing_policies/attachments/conftest.py (1)
4-6: Consider using direct dictionary access for fail-fast behavior.Using
.get()silently returnsNoneif the config key is missing, which could lead to confusing test failures. Direct access with[]would raise a clearKeyErrorimmediately if the configuration is incomplete.@pytest.fixture def attachment_id(e2e_config): - return e2e_config.get("catalog.pricing_policy.attachment.id") + return e2e_config["catalog.pricing_policy.attachment.id"]tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py (3)
14-22: Remove unusedloggerparameter or use it for teardown logging.The
loggerparameter is declared but never used. The teardown usesprint()instead. Either remove the parameter or use it consistently.Option 1 - Remove unused parameter:
@pytest.fixture -def created_attachment(logger, attachment_service, attachment_data, pdf_fd): +def created_attachment(attachment_service, attachment_data, pdf_fd): attachment = attachment_service.create(attachment_data, file=pdf_fd) yield attachment try: attachment_service.delete(attachment.id) except MPTAPIError as error: print(f"TEARDOWN - Unable to delete attachment {attachment.id}: {error.title}")Option 2 - Use logger consistently:
except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete attachment {attachment.id}: {error.title}") + logger.warning(f"TEARDOWN - Unable to delete attachment {attachment.id}: {error.title}")
57-59: Consider using a valid ID prefix pattern for the not-found test.The ID
"ATT-000-000-000"doesn't follow thePPA-*pattern used for pricing policy attachments elsewhere in the config. Using"PPA-0000-0000-0000-0000"would be more consistent and realistic.def test_not_found(attachment_service): with pytest.raises(MPTAPIError): - attachment_service.get("ATT-000-000-000") + attachment_service.get("PPA-0000-0000-0000-0000")
62-65: Potential double-delete:created_attachmentfixture will attempt cleanup after test already deletes.The
test_delete_attachmenttest explicitly deletes the attachment, then thecreated_attachmentfixture teardown attempts to delete it again. While the teardown handles the error gracefully, this logs a spurious error message. Consider marking the fixture to skip cleanup when intentionally deleted:@pytest.fixture def created_attachment(attachment_service, attachment_data, pdf_fd): attachment = attachment_service.create(attachment_data, file=pdf_fd) attachment._deleted = False # Track deletion state yield attachment if not getattr(attachment, '_deleted', False): try: attachment_service.delete(attachment.id) except MPTAPIError as error: print(f"TEARDOWN - Unable to delete attachment {attachment.id}: {error.title}")Alternatively, this is acceptable as-is since the error is caught.
tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py (1)
14-22: Use the injectedloggerinstead ofThe
loggerfixture is injected but never used, and teardown logs go throughYou can both fix the warning and improve logging by switching to
logger.warning(or similar) in the teardown:@pytest.fixture async def created_attachment_async(logger, async_attachment_service, attachment_data, pdf_fd): attachment = await async_attachment_service.create(attachment_data, file=pdf_fd) yield attachment try: await async_attachment_service.delete(attachment.id) except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete attachment {attachment.id}: {error.title}") + logger.warning( + "TEARDOWN - Unable to delete attachment %s: %s", + attachment.id, + error.title, + )Note: this also makes the “unable to delete” message clearly come from test teardown, which is helpful given that
test_delete_attachment_asyncwill intentionally delete the resource before fixture teardown runs.
📜 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/catalog/pricing_policy_attachments.py(3 hunks)tests/e2e/catalog/pricing_policies/attachments/conftest.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py(1 hunks)tests/unit/resources/catalog/test_pricing_policy_attachments.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/catalog/pricing_policies/attachments/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
mpt_api_client/resources/catalog/pricing_policy_attachments.py (3)
mpt_api_client/http/mixins.py (2)
AsyncDownloadFileMixin(231-253)DownloadFileMixin(59-81)mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/resources/catalog/mixins.py (2)
AsyncCreateFileMixin(84-110)CreateFileMixin(121-147)
🪛 Ruff (0.14.5)
tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py
15-15: Unused function argument: logger
(ARG001)
tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py
15-15: Unused function argument: logger
(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 (8)
e2e_config.test.json (1)
22-24: LGTM!The new configuration entries follow the existing naming conventions and ID patterns. The pricing policy attachment ID (
PPA-1123-5132-3456-0001) appropriately includes the parent pricing policy ID prefix.tests/e2e/catalog/pricing_policies/attachments/conftest.py (1)
9-14: LGTM!The fixture provides reusable test data with clear, descriptive values.
tests/unit/resources/catalog/test_pricing_policy_attachments.py (1)
39-46: LGTM!The updated method list correctly reflects the refactored service interface—
downloadfromDownloadFileMixinandcreatefromCreateFileMixin, withactivate/deactivateappropriately removed.tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py (1)
24-54: LGTM!The test coverage is comprehensive, covering CRUD operations, iteration, filtering, and error handling. The assertions are appropriate and verify the expected behavior.
mpt_api_client/resources/catalog/pricing_policy_attachments.py (3)
1-14: LGTM!The import restructuring cleanly separates concerns—
DownloadFileMixinfrom generic HTTP mixins andCreateFileMixinfrom catalog-specific mixins. This aligns with the different behaviors needed for file operations.
21-28: LGTM!The configuration class properly defines
_upload_file_keyand_upload_data_keywhich are consumed byCreateFileMixinfor multipart form construction.
31-50: LGTM!The service classes correctly inherit from the new mixins with proper MRO ordering. The removal of
ActivatableMixinand replacement with explicitCreateFileMixin/DownloadFileMixinprovides clearer, more precise file operation capabilities.tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py (1)
29-66: Async lifecycle tests for attachments look comprehensiveThe async tests for update, get, download, iteration, filtering, not‑found, and delete exercise the key behaviors of the attachments service (including iterator + RQL query usage and error handling) and read clearly. No issues from my side here.
| def test_create_attachment_async(created_attachment_async, attachment_data): | ||
| assert created_attachment_async.name == attachment_data["name"] | ||
| assert created_attachment_async.description == attachment_data["description"] | ||
|
|
There was a problem hiding this comment.
test_create_attachment_async should likely be async def to match the async fixture
All other tests using created_attachment_async are async def, but this one is a plain def. With an async fixture, this can result in the test receiving a coroutine instead of the resolved attachment object (depending on your async plugin configuration), and created_attachment_async.name would then fail.
Recommend aligning it with the rest of the suite:
-def test_create_attachment_async(created_attachment_async, attachment_data):
+async def test_create_attachment_async(created_attachment_async, attachment_data):
assert created_attachment_async.name == attachment_data["name"]
assert created_attachment_async.description == attachment_data["description"]This keeps usage consistent and avoids potential coroutine/attribute errors at runtime.
🤖 Prompt for AI Agents
In tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py
around lines 24 to 27, the test function is declared as a synchronous def but
uses the async fixture created_attachment_async; change the test to be async def
so pytest/async plugin will await the fixture and provide the resolved
attachment, then keep the same assertions (i.e., make the function async to
avoid receiving a coroutine and attribute access errors).
4d263d8 to
89ffd3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/catalog/pricing_policies/attachments/conftest.py (1)
24-31: Use the injectedloggerinstead of
loggeris currently unused and teardown errors are printed instead of logged. You can both fix the Ruff warning and improve logging by wiring the logger into the exception path:-@pytest.fixture -def created_attachment(logger, attachment_service, attachment_data, pdf_fd): +@pytest.fixture +def created_attachment(logger, attachment_service, attachment_data, pdf_fd): attachment = attachment_service.create(attachment_data, file=pdf_fd) yield attachment try: attachment_service.delete(attachment.id) except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete attachment {attachment.id}: {error.title}") + logger.warning( + "TEARDOWN - Unable to delete attachment %s: %s", + attachment.id, + error.title, + )tests/e2e/catalog/pricing_policies/conftest.py (1)
23-37: Align teardown logging with the sharedloggerfixture (optional)The teardown currently uses
loggerfixture here and logging a warning instead:-@pytest.fixture -def created_pricing_policy(pricing_policies_service, pricing_policy_data): +@pytest.fixture +def created_pricing_policy(logger, pricing_policies_service, pricing_policy_data): policy = pricing_policies_service.create(pricing_policy_data) yield policy try: pricing_policies_service.delete(policy.id) except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete pricing policy {policy.id}: {error.title}") + logger.warning( + "TEARDOWN - Unable to delete pricing policy %s: %s", + policy.id, + error.title, + )tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py (1)
14-21: Use theloggerfixture instead ofSame as the sync attachments fixture:
loggeris injected but unused, and teardown failures are printed. Switching to the logger both fixes the Ruff warning and improves observability.-@pytest.fixture -async def created_attachment_async(logger, async_attachment_service, attachment_data, pdf_fd): +@pytest.fixture +async def created_attachment_async(logger, async_attachment_service, attachment_data, pdf_fd): attachment = await async_attachment_service.create(attachment_data, file=pdf_fd) yield attachment try: await async_attachment_service.delete(attachment.id) except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete attachment {attachment.id}: {error.title}") + logger.warning( + "TEARDOWN - Unable to delete attachment %s: %s", + attachment.id, + error.title, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
mpt_api_client/resources/catalog/pricing_policy_attachments.py(3 hunks)tests/e2e/catalog/pricing_policies/attachments/conftest.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py(1 hunks)tests/e2e/catalog/pricing_policies/conftest.py(2 hunks)tests/e2e/catalog/pricing_policies/test_async_pricing_policies.py(1 hunks)tests/e2e/catalog/pricing_policies/test_sync_pricing_policies.py(0 hunks)tests/unit/resources/catalog/test_pricing_policy_attachments.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/catalog/pricing_policies/test_sync_pricing_policies.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/resources/catalog/test_pricing_policy_attachments.py
- tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/catalog/pricing_policies/attachments/test_async_attachment.py
🧬 Code graph analysis (4)
tests/e2e/catalog/pricing_policies/attachments/conftest.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/conftest.py (3)
mpt_ops(31-32)logger(63-64)pdf_fd(73-75)tests/e2e/catalog/pricing_policies/conftest.py (1)
pricing_policy_id(41-42)
mpt_api_client/resources/catalog/pricing_policy_attachments.py (2)
mpt_api_client/http/mixins.py (2)
AsyncDownloadFileMixin(231-253)DownloadFileMixin(59-81)mpt_api_client/resources/catalog/mixins.py (2)
AsyncCreateFileMixin(84-110)CreateFileMixin(121-147)
tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)pdf_fd(73-75)tests/e2e/catalog/pricing_policies/conftest.py (1)
pricing_policy_id(41-42)tests/e2e/catalog/pricing_policies/attachments/conftest.py (2)
attachment_data(12-16)attachment_id(7-8)
tests/e2e/catalog/pricing_policies/conftest.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/unit/resources/catalog/test_pricing_policies.py (1)
pricing_policies_service(16-17)tests/e2e/conftest.py (1)
mpt_ops(31-32)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.6)
tests/e2e/catalog/pricing_policies/attachments/conftest.py
25-25: Unused function argument: logger
(ARG001)
tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py
15-15: Unused function argument: logger
(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 (3)
tests/e2e/catalog/pricing_policies/test_async_pricing_policies.py (1)
35-37: Async get-by-id test wiring looks correctUsing
pricing_policy_idhere is a nice way to verify the async service can retrieve a policy created via the sync fixture; the assertions are minimal and sufficient.mpt_api_client/resources/catalog/pricing_policy_attachments.py (1)
11-48: Create/download mixin wiring and upload config look consistentThe switch to
CreateFileMixin/AsyncCreateFileMixinandDownloadFileMixin/AsyncDownloadFileMixinwith_upload_file_key = "file"and_upload_data_key = "attachment"inPricingPolicyAttachmentsServiceConfigis consistent with the shared mixin implementations and keeps both sync and async services aligned.tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py (1)
24-27: Sync test using async fixture is fine with your pytest-asyncio setup
test_create_attachment_asyncis a regulardefbut consumes the asynccreated_attachment_asyncfixture. With pytest-asyncio automatically resolving async fixtures before injection, this pattern is valid, and the assertions read cleanly without extraawaitnoise. Based on learnings, this does not need to be converted toasync def.
albertsola
force-pushed
the
e2e/catalog/MPT-14900-pricing-policy-attachments
branch
from
89ffd3a to
5a5e2b4
Compare
albertsola
changed the title
|
Please, do me the favor of following the AAA pattern we already have here -> https://github.com/softwareone-platform/mpt-api-python-client/pull/134/files And save me the work to fixing it once you merge the PR |
albertsola
force-pushed
the
e2e/catalog/MPT-14900-pricing-policy-attachments
branch
from
5a5e2b4 to
994f471
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/catalog/product/media/test_sync_media.py (1)
34-42:test_create_media_from_urlis not using the URL-based fixtureThe test named
test_create_media_from_urlcurrently depends oncreated_media_from_file, so it never exercises creation via URL and thecreated_media_from_urlfixture is unused.Consider wiring the test to the correct fixture so the URL path is actually covered:
-def test_create_media_from_url(created_media_from_file, media_data): # noqa: AAA01 - assert created_media_from_file.name == media_data["name"] - assert created_media_from_file.description == media_data["description"] +def test_create_media_from_url(created_media_from_url, media_data): # noqa: AAA01 + assert created_media_from_url.name == media_data["name"] + assert created_media_from_url.description == media_data["description"]
🧹 Nitpick comments (1)
tests/e2e/catalog/pricing_policies/conftest.py (1)
40-42: Consider async variant for async tests.While this fixture correctly extracts the ID from
created_pricing_policy, it creates a synchronous dependency chain for async tests (as seen intest_async_pricing_policies.pyline 38 where the async test now uses this fixture).For consistency with the async test execution model, consider whether async tests should use an
async_pricing_policy_idfixture derived fromasync_created_pricing_policyinstead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
mpt_api_client/resources/catalog/pricing_policy_attachments.py(3 hunks)tests/e2e/catalog/items/test_async_item.py(1 hunks)tests/e2e/catalog/items/test_sync_item.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/conftest.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py(1 hunks)tests/e2e/catalog/pricing_policies/conftest.py(2 hunks)tests/e2e/catalog/pricing_policies/test_async_pricing_policies.py(1 hunks)tests/e2e/catalog/pricing_policies/test_sync_pricing_policies.py(0 hunks)tests/e2e/catalog/product/documents/test_async_document.py(2 hunks)tests/e2e/catalog/product/documents/test_sync_document.py(2 hunks)tests/e2e/catalog/product/item_groups/test_async_item_groups.py(1 hunks)tests/e2e/catalog/product/item_groups/test_sync_item_groups.py(1 hunks)tests/e2e/catalog/product/media/test_async_media.py(2 hunks)tests/e2e/catalog/product/media/test_sync_media.py(2 hunks)tests/e2e/catalog/product/parameter_groups/test_async_parameter_groups.py(1 hunks)tests/e2e/catalog/product/parameter_groups/test_sync_parameter_groups.py(1 hunks)tests/e2e/catalog/product/terms/test_sync_terms.py(2 hunks)tests/e2e/catalog/product/terms/variants/test_sync_variants.py(2 hunks)tests/e2e/catalog/product/test_async_product.py(1 hunks)tests/e2e/catalog/product/test_sync_product.py(1 hunks)tests/unit/resources/catalog/test_pricing_policy_attachments.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/catalog/pricing_policies/test_sync_pricing_policies.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/catalog/product/parameter_groups/test_sync_parameter_groups.py
- tests/e2e/catalog/product/terms/test_sync_terms.py
- tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py
- tests/e2e/catalog/product/terms/variants/test_sync_variants.py
- tests/e2e/catalog/product/test_async_product.py
- tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/catalog/items/test_async_item.pytests/e2e/catalog/product/documents/test_async_document.pytests/e2e/catalog/product/parameter_groups/test_async_parameter_groups.pytests/e2e/catalog/product/media/test_async_media.py
🧬 Code graph analysis (12)
tests/e2e/catalog/items/test_async_item.py (2)
tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/catalog/items/conftest.py (1)
item_data(5-20)
tests/e2e/catalog/product/documents/test_async_document.py (3)
tests/unit/resources/catalog/test_mixins.py (1)
async_document_service(354-355)tests/e2e/catalog/product/documents/conftest.py (1)
document_data(10-16)tests/e2e/conftest.py (2)
pdf_fd(73-75)pdf_url(79-80)
tests/e2e/catalog/product/item_groups/test_sync_item_groups.py (2)
tests/e2e/conftest.py (2)
mpt_vendor(19-20)product_id(96-97)tests/e2e/catalog/product/item_groups/conftest.py (1)
item_group_data(5-15)
tests/e2e/catalog/items/test_sync_item.py (2)
tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/catalog/items/conftest.py (1)
item_data(5-20)
tests/e2e/catalog/product/item_groups/test_async_item_groups.py (2)
tests/e2e/conftest.py (2)
async_mpt_vendor(24-27)product_id(96-97)tests/e2e/catalog/product/item_groups/conftest.py (1)
item_group_data(5-15)
tests/e2e/catalog/product/documents/test_sync_document.py (2)
tests/e2e/catalog/product/documents/conftest.py (2)
vendor_document_service(20-21)document_data(10-16)tests/e2e/conftest.py (2)
pdf_fd(73-75)pdf_url(79-80)
tests/e2e/catalog/product/media/test_async_media.py (3)
tests/unit/http/test_mixins.py (1)
async_media_service(35-38)tests/e2e/catalog/product/media/conftest.py (2)
media_data(5-14)test_media_file(18-19)tests/e2e/conftest.py (1)
jpg_url(84-85)
tests/e2e/catalog/pricing_policies/attachments/conftest.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/conftest.py (2)
mpt_ops(31-32)pdf_fd(73-75)tests/e2e/catalog/pricing_policies/conftest.py (1)
pricing_policy_id(41-42)
tests/e2e/catalog/product/test_sync_product.py (2)
tests/e2e/conftest.py (2)
mpt_vendor(19-20)logo_fd(106-108)tests/e2e/catalog/product/conftest.py (1)
product_data(5-6)
tests/e2e/catalog/pricing_policies/conftest.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/unit/resources/catalog/test_pricing_policies.py (1)
pricing_policies_service(16-17)tests/e2e/conftest.py (1)
mpt_ops(31-32)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/catalog/product/media/test_sync_media.py (2)
tests/e2e/catalog/product/media/conftest.py (2)
media_data(5-14)test_media_file(18-19)tests/e2e/conftest.py (1)
jpg_url(84-85)
mpt_api_client/resources/catalog/pricing_policy_attachments.py (2)
mpt_api_client/http/mixins.py (2)
AsyncDownloadFileMixin(231-253)DownloadFileMixin(59-81)mpt_api_client/resources/catalog/mixins.py (2)
AsyncCreateFileMixin(84-110)CreateFileMixin(121-147)
🔇 Additional comments (20)
tests/unit/resources/catalog/test_pricing_policy_attachments.py (1)
43-43: LGTM!The parameterized methods list correctly reflects the service's new API surface after replacing
ActivatableMixinwithCreateFileMixinandDownloadFileMixin.Also applies to: 50-50
tests/e2e/catalog/items/test_async_item.py (1)
9-17: LGTM!Removing the unused
loggerparameter simplifies the fixture signature without affecting functionality.tests/e2e/catalog/product/test_sync_product.py (1)
7-16: LGTM!Consistent with the PR-wide cleanup of unused
loggerparameters from fixtures.tests/e2e/catalog/product/documents/test_async_document.py (1)
14-33: LGTM!Both fixtures (
created_document_from_file_asyncandcreated_document_from_link_async) have their unusedloggerparameters removed, consistent with the PR-wide cleanup.tests/e2e/catalog/product/parameter_groups/test_async_parameter_groups.py (1)
6-14: LGTM!Removing the unused
loggerparameter aligns with the PR-wide fixture signature cleanup.mpt_api_client/resources/catalog/pricing_policy_attachments.py (2)
21-28: Configuration keys look correct for file upload semantics.The
_upload_file_key = "file"and_upload_data_key = "attachment"will be used byCreateFileMixinto structure multipart requests. Ensure these keys match the API's expected field names for pricing policy attachments.
31-50: Well-structured mixin composition.The service classes correctly compose the file-operations mixins (
CreateFileMixin,DownloadFileMixin) with the resource management mixins. The MRO places mixins before the baseServiceclass, ensuring proper method resolution. This aligns with the updated unit test expectations.tests/e2e/catalog/product/item_groups/test_async_item_groups.py (1)
9-17: Async item group fixture change looks correctRemoving the unused
loggerparameter keeps the fixture clean, and the async create/delete flow remains unchanged and consistent with the sync variant.tests/e2e/catalog/items/test_sync_item.py (1)
9-17: Synchronous item fixture refactor is safeDropping the
loggerparameter fromcreated_itemis harmless here—the body never used it, and create/teardown behavior is preserved.tests/e2e/catalog/product/item_groups/test_sync_item_groups.py (1)
9-17: Sync item group fixture cleanup looks goodThe
created_item_groupfixture no longer accepts an unusedlogger, while creation and teardown semantics remain intact and aligned with other catalog fixtures.tests/e2e/catalog/product/media/test_sync_media.py (1)
13-31: Media fixtures simplified correctlyBoth
created_media_from_fileandcreated_media_from_urlno longer take alogger, and their create/yield/delete flows are unchanged, so behavior and teardown guarantees remain the same.tests/e2e/catalog/product/media/test_async_media.py (1)
18-36: Async media fixtures are structured correctly after logger removalThe async fixtures still
awaitcreate/delete and yield the media objects, and tests use the fixtures directly without awaiting them, which matches pytest-asyncio fixture semantics. Removing the unusedloggerargument is safe here. Based on learnings, async fixtures are being used as intended.tests/e2e/catalog/product/documents/test_sync_document.py (1)
9-28: Document fixtures logger removal is non-disruptive
created_document_from_fileandcreated_document_from_urlno longer accept alogger, but their create and teardown logic is unchanged, so existing tests keep the same behavior with a simpler fixture surface.tests/e2e/catalog/pricing_policies/attachments/conftest.py (4)
6-8: LGTM!This fixture follows the standard pattern of extracting a primitive ID from a created resource, which is useful for tests that only need the identifier.
11-16: LGTM!The attachment data structure is appropriate, and the "please delete" naming convention helps identify orphaned test resources.
19-21: LGTM!The fixture correctly scopes the attachment service to a specific pricing policy using the
pricing_policy_idparameter, which aligns with the service architecture.
24-31: The original review comment is incorrect. Async attachment tests already have corresponding async fixtures that are properly implemented.The
created_attachment_asyncfixture is defined intest_async_attachment.py(lines 16-22) with proper async/await semantics for setup and teardown. Per pytest-asyncio behavior (confirmed by learnings), async fixtures are automatically resolved before being passed to test functions—both sync and async. The sync test functiontest_create_attachment_asynccorrectly receives the resolved fixture value without requiring any manual awaiting. The fixture organization and implementation is correct and requires no changes.tests/e2e/catalog/pricing_policies/conftest.py (3)
3-3: LGTM!The
MPTAPIErrorimport is correctly added to support exception handling during teardown.
24-25: LGTM!The fixture follows the standard pattern for accessing the pricing policies service from the MPT client.
28-37: I'll verify the concerns raised in the review comment. Let me start by investigating the actual state of the fixtures and the AAA pattern mentioned.
<function_calls>
#!/bin/bashFind async fixture in pricing policies tests
find . -name "pricing" -type f | head -20
</function_calls>
<function_calls>
#!/bin/bashSearch for async_created_pricing_policy fixture
rg "async_created_pricing_policy" -n -A 5 -B 2
</function_calls>
<function_calls>
#!/bin/bashSearch for all pricing policy fixtures
rg "@pytest.fixture" -A 3 tests/e2e/catalog/pricing_policies/
</function_calls>Now let me check the GitHub PR #134 to understand the AAA pattern context:
<function_calls>
GitHub softwareone-platform/mpt-api-python-client PR 134 AAA pattern
</function_calls>
| async def test_get_pricing_policy_by_id(async_pricing_policies_service, pricing_policy_id): | ||
| result = await async_pricing_policies_service.get(pricing_policy_id) | ||
|
|
||
| assert result.id == async_created_pricing_policy.id | ||
| assert result.id == pricing_policy_id |
There was a problem hiding this comment.
Inconsistent fixture usage in async test.
This async test now uses pricing_policy_id, which depends on the synchronous created_pricing_policy fixture (from conftest.py). However, the file defines its own async_created_pricing_policy fixture (lines 15-23), and other async tests in this file still use it (e.g., lines 32-35, 44-51).
This creates an inconsistency where async tests have mixed dependencies on both sync and async fixtures. For consistency and clarity, async tests should use async fixtures.
Based on the reviewer's comment about following the AAA pattern from PR #134, consider reverting this change to use async_created_pricing_policy instead, maintaining consistency across async tests.
-async def test_get_pricing_policy_by_id(async_pricing_policies_service, pricing_policy_id):
- result = await async_pricing_policies_service.get(pricing_policy_id)
+async def test_get_pricing_policy_by_id(async_pricing_policies_service, async_created_pricing_policy):
+ result = await async_pricing_policies_service.get(async_created_pricing_policy.id)
- assert result.id == pricing_policy_id
+ assert result.id == async_created_pricing_policy.idCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/e2e/catalog/pricing_policies/test_async_pricing_policies.py around
lines 38 to 41, the async test currently depends on the synchronous fixture
pricing_policy_id creating an inconsistent mix of sync and async fixtures;
change the test to depend on the async_created_pricing_policy fixture instead
(accept async_created_pricing_policy as the parameter) and use its id (e.g.,
async_created_pricing_policy.id) when calling async_pricing_policies_service.get
so the test consistently uses async fixtures like the other tests in this file.
994f471 to
518047f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/catalog/product/media/test_sync_media.py (1)
84-86: Fix async/sync mismatch in sync test file.This test is defined as
async defand usesawait, butvendor_media_serviceis a synchronous service. This will cause a runtime error.Apply this diff to fix the issue:
-async def test_get_not_found_media(vendor_media_service): +def test_get_not_found_media(vendor_media_service): with pytest.raises(MPTAPIError, match=r"404 Not Found"): - await vendor_media_service.get("INVALID-ID") + vendor_media_service.get("INVALID-ID")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
mpt_api_client/resources/catalog/pricing_policy_attachments.py(3 hunks)tests/e2e/catalog/items/test_async_item.py(1 hunks)tests/e2e/catalog/items/test_sync_item.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/conftest.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py(1 hunks)tests/e2e/catalog/pricing_policies/conftest.py(2 hunks)tests/e2e/catalog/pricing_policies/test_async_pricing_policies.py(1 hunks)tests/e2e/catalog/pricing_policies/test_sync_pricing_policies.py(0 hunks)tests/e2e/catalog/product/documents/test_async_document.py(2 hunks)tests/e2e/catalog/product/documents/test_sync_document.py(2 hunks)tests/e2e/catalog/product/item_groups/test_async_item_groups.py(1 hunks)tests/e2e/catalog/product/item_groups/test_sync_item_groups.py(1 hunks)tests/e2e/catalog/product/media/test_async_media.py(2 hunks)tests/e2e/catalog/product/media/test_sync_media.py(2 hunks)tests/e2e/catalog/product/parameter_groups/test_async_parameter_groups.py(1 hunks)tests/e2e/catalog/product/parameter_groups/test_sync_parameter_groups.py(1 hunks)tests/e2e/catalog/product/terms/test_sync_terms.py(2 hunks)tests/e2e/catalog/product/terms/variants/test_sync_variants.py(2 hunks)tests/e2e/catalog/product/test_async_product.py(1 hunks)tests/e2e/catalog/product/test_sync_product.py(1 hunks)tests/unit/resources/catalog/test_pricing_policy_attachments.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/catalog/pricing_policies/test_sync_pricing_policies.py
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/e2e/catalog/product/item_groups/test_sync_item_groups.py
- tests/e2e/catalog/items/test_sync_item.py
- tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py
- tests/e2e/catalog/product/item_groups/test_async_item_groups.py
- tests/e2e/catalog/pricing_policies/conftest.py
- tests/e2e/catalog/product/documents/test_sync_document.py
- tests/e2e/catalog/product/documents/test_async_document.py
- tests/e2e/catalog/pricing_policies/test_async_pricing_policies.py
- tests/e2e/catalog/product/terms/variants/test_sync_variants.py
- tests/unit/resources/catalog/test_pricing_policy_attachments.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/catalog/product/media/test_async_media.pytests/e2e/catalog/product/parameter_groups/test_async_parameter_groups.pytests/e2e/catalog/items/test_async_item.pytests/e2e/catalog/product/test_async_product.py
🧬 Code graph analysis (8)
tests/e2e/catalog/pricing_policies/attachments/conftest.py (3)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/conftest.py (2)
mpt_ops(31-32)pdf_fd(73-75)
tests/e2e/catalog/product/parameter_groups/test_sync_parameter_groups.py (2)
tests/e2e/conftest.py (2)
mpt_vendor(19-20)product_id(96-97)tests/e2e/catalog/product/parameter_groups/conftest.py (1)
parameter_group_data(5-10)
tests/e2e/catalog/product/parameter_groups/test_async_parameter_groups.py (2)
tests/e2e/conftest.py (2)
async_mpt_vendor(24-27)product_id(96-97)tests/e2e/catalog/product/parameter_groups/conftest.py (1)
parameter_group_data(5-10)
tests/e2e/catalog/items/test_async_item.py (2)
tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/catalog/items/conftest.py (1)
item_data(5-20)
tests/e2e/catalog/product/terms/test_sync_terms.py (2)
tests/e2e/catalog/product/terms/conftest.py (1)
term_data(5-6)tests/e2e/conftest.py (1)
pdf_url(79-80)
tests/e2e/catalog/product/media/test_sync_media.py (2)
tests/e2e/catalog/product/media/conftest.py (2)
media_data(5-14)test_media_file(18-19)tests/e2e/conftest.py (1)
jpg_url(84-85)
tests/e2e/catalog/product/test_sync_product.py (2)
tests/e2e/conftest.py (2)
mpt_vendor(19-20)logo_fd(106-108)tests/e2e/catalog/product/conftest.py (1)
product_data(5-6)
tests/e2e/catalog/product/test_async_product.py (2)
tests/e2e/conftest.py (2)
async_mpt_vendor(24-27)logo_fd(106-108)tests/e2e/catalog/product/conftest.py (1)
product_data(5-6)
⏰ 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 (24)
mpt_api_client/resources/catalog/pricing_policy_attachments.py (4)
1-14: Clean import organization with clear separation of concerns.The imports are well-organized, separating generic HTTP mixins from domain-specific catalog mixins. The choice to import
CreateFileMixinfrom the catalog module suggests domain-specific upload logic, whileDownloadFileMixinremains generic.
21-28: Configuration properly centralizes upload keys for the new file mixins.The addition of
_upload_file_keyand_upload_data_keyaligns with the newCreateFileMixinrequirements. The configuration class pattern ensures consistency between sync and async service variants.
31-39: LGTM!The inheritance order follows the correct MRO pattern: specific mixins first (file operations), then CRUD mixins, base service, and configuration last. This ensures proper method resolution and attribute availability.
42-50: LGTM!The async service maintains perfect parity with its sync counterpart, reusing the shared configuration class. The inheritance structure and type parameters are consistent.
tests/e2e/catalog/product/test_async_product.py (1)
8-8: LGTM: Fixture signature simplified.The removal of the unused
loggerparameter streamlines the fixture signature without affecting functionality.tests/e2e/catalog/items/test_async_item.py (1)
10-10: LGTM: Fixture signature simplified.The removal of the unused
loggerparameter is consistent with the cleanup pattern across the test suite.tests/e2e/catalog/product/media/test_async_media.py (1)
19-19: LGTM: Fixture signatures simplified.Both fixture signatures have been appropriately simplified by removing the unused
loggerparameter.Also applies to: 29-29
tests/e2e/catalog/product/test_sync_product.py (1)
8-8: LGTM: Fixture signature simplified.The removal of the unused
loggerparameter is appropriate and consistent with the sync test pattern.tests/e2e/catalog/product/terms/test_sync_terms.py (1)
15-15: LGTM: Fixture signatures simplified.Both fixtures have been appropriately updated by removing the unused
loggerparameter.Also applies to: 26-26
tests/e2e/catalog/product/parameter_groups/test_sync_parameter_groups.py (1)
7-7: LGTM: Fixture signature simplified.The logger parameter removal is appropriate and maintains consistency across the test suite.
tests/e2e/catalog/product/parameter_groups/test_async_parameter_groups.py (1)
7-7: LGTM: Fixture signature simplified.The removal of the unused
loggerparameter aligns with the async test fixture pattern.tests/e2e/catalog/product/media/test_sync_media.py (1)
14-14: LGTM: Fixture signatures simplified.Both fixtures have been appropriately updated by removing the unused
loggerparameter.Also applies to: 24-24
tests/e2e/catalog/pricing_policies/attachments/conftest.py (4)
6-8: LGTM!Standard fixture pattern for extracting the ID from the created attachment resource.
11-16: LGTM!Test data fixture is well-structured with clear test indicators in the values.
19-21: LGTM!Standard service accessor fixture following the established pattern.
24-31: LGTM!Standard lifecycle fixture with appropriate teardown error handling for E2E tests. The error message in the teardown provides helpful debugging context.
tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py (8)
16-21: LGTM!This test follows the AAA pattern correctly with an explicit Act step and clear assertions.
24-27: LGTM!Clean test with explicit Act step and appropriate assertion.
30-34: LGTM!Test follows AAA pattern with explicit download operation and verifies both content and filename.
37-40: LGTM!Test follows AAA pattern with explicit iteration and appropriate membership assertion.
43-47: LGTM!Test follows AAA pattern with explicit filter operation and verifies both result count and content.
50-52: LGTM!Appropriate use of
pytest.raisesfor exception testing. The combined Act+Assert pattern is standard for negative test cases.
55-59: LGTM!Test follows AAA pattern with explicit delete operation and appropriate verification that the resource is no longer accessible.
6-6: LGTM!Appropriate use of
pytest.mark.flakyfor E2E tests that may experience intermittent failures.
| def test_create_attachment(created_attachment, attachment_data): | ||
| result = created_attachment | ||
|
|
||
| assert result.name == attachment_data["name"] | ||
| assert result.description == attachment_data["description"] |
There was a problem hiding this comment.
Refactor to follow AAA (Arrange-Act-Assert) pattern.
This test doesn't follow the AAA pattern requested by the reviewer. The "Act" step (creating the attachment) is hidden in the created_attachment fixture, making it unclear what operation is being tested. The test should explicitly call attachment_service.create() to make the action visible.
Based on the reviewer's feedback, consider this structure:
-def test_create_attachment(created_attachment, attachment_data):
- result = created_attachment
+def test_create_attachment(attachment_service, attachment_data, pdf_fd):
+ # Act
+ result = attachment_service.create(attachment_data, file=pdf_fd)
+ # Assert
assert result.name == attachment_data["name"]
assert result.description == attachment_data["description"]
+
+ # Cleanup
+ try:
+ attachment_service.delete(result.id)
+ except MPTAPIError:
+ passCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py around
lines 9 to 13, the test hides the "Act" step by using the created_attachment
fixture; update the test to follow Arrange-Act-Assert by arranging any needed
dependencies (e.g., attachment_service and attachment_data fixtures), explicitly
calling the action under test (call attachment_service.create(...) with
attachment_data) as the Act, and then assert on the returned result's name and
description; remove reliance on the created_attachment fixture or rename it to a
helper, and keep asserts the same.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch PR #134 to see the AAA pattern being referenced
gh pr view 134 --repo softwareone-platform/mpt-api-python-client --json files --jq '.files[].path' | head -20
Length of output: 910
🏁 Script executed:
#!/bin/bash
# Get the actual test file content from PR #134 to see the AAA pattern
gh pr diff 134 --repo softwareone-platform/mpt-api-python-client | head -300
Length of output: 10072
@jentyk You're absolutely right! I apologize for the confusion. After reviewing PR #134, I can see that the AAA pattern they use does rely on fixtures for setup and teardown.
Looking at examples from PR #134:
def test_create_account(created_account):
result = created_account
assert result is not NoneThe current test in this PR already follows that same pattern:
def test_create_attachment(created_attachment, attachment_data):
result = created_attachment
assert result.name == attachment_data["name"]
assert result.description == attachment_data["description"]The test correctly:
- ✅ Uses the fixture for arrangement (setup)
- ✅ Assigns to
resultvariable (act) - ✅ Has a blank line separating act from assert
- ✅ Has clear assertions
- ✅ Relies on fixture teardown for cleanup
My previous suggestion to add explicit cleanup and remove the fixture was incorrect. The test structure is actually fine as-is. Thank you for catching that!
albertsola
force-pushed
the
e2e/catalog/MPT-14900-pricing-policy-attachments
branch
from
518047f to
90761ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/catalog/product/media/test_sync_media.py (2)
39-42:test_create_media_from_urlis not using the URL-based fixtureThis test currently uses
created_media_from_file, so it never verifies the URL-based creation path even though the name suggests it does.Consider switching to the URL fixture:
-def test_create_media_from_url(created_media_from_file, media_data): # noqa: AAA01 - assert created_media_from_file.name == media_data["name"] - assert created_media_from_file.description == media_data["description"] +def test_create_media_from_url(created_media_from_url, media_data): # noqa: AAA01 + assert created_media_from_url.name == media_data["name"] + assert created_media_from_url.description == media_data["description"][scratchpad_end] -->
84-86: Async test incorrectly awaits a synchronous service
vendor_media_serviceis a synchronous client, buttest_get_not_found_mediaisasyncand awaitsvendor_media_service.get(...), which is not awaitable. This is a runtime bug if the test runs.You can make this a regular sync test:
-async def test_get_not_found_media(vendor_media_service): - with pytest.raises(MPTAPIError, match=r"404 Not Found"): - await vendor_media_service.get("INVALID-ID") +def test_get_not_found_media(vendor_media_service): + with pytest.raises(MPTAPIError, match=r"404 Not Found"): + vendor_media_service.get("INVALID-ID")
♻️ Duplicate comments (1)
tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py (1)
9-13: Refactortest_create_attachmentto follow AAA explicitlyHere the "Act" step (creating the attachment) is hidden inside the
created_attachmentfixture. Given the reviewer’s request to follow the AAA pattern, this test should explicitly callattachment_service.create(...)so the operation under test is visible, and then perform its assertions and cleanup.For example:
-def test_create_attachment(created_attachment, attachment_data): - result = created_attachment - - assert result.name == attachment_data["name"] - assert result.description == attachment_data["description"] +def test_create_attachment(attachment_service, attachment_data, pdf_fd): + # Act + result = attachment_service.create(attachment_data, file=pdf_fd) + + # Assert + assert result.name == attachment_data["name"] + assert result.description == attachment_data["description"] + + # Cleanup + try: + attachment_service.delete(result.id) + except MPTAPIError: + passThis keeps the test self-contained and makes the action under test obvious while still ensuring cleanup.
🧹 Nitpick comments (2)
tests/e2e/catalog/product/terms/variants/test_sync_variants.py (1)
25-33: Consider avoiding in‑place mutation ofvariant_datainside the fixtureHere
created_variant_from_urlmutates the sharedvariant_datafixture (type,assetUrl). Because pytest shares a single fixture instance within a test, thevariant_dataargument in the test function will see these mutations, which is a bit subtle and could surprise future assertions on those fields. Behavior is correct today (tests only usename), but you might consider copying before mutation:-@pytest.fixture -def created_variant_from_url(vendor_variant_service, variant_data, pdf_url): - variant_data["type"] = "Online" - variant_data["assetUrl"] = pdf_url - variant = vendor_variant_service.create(variant_data) +@pytest.fixture +def created_variant_from_url(vendor_variant_service, variant_data, pdf_url): + data = {**variant_data} + data["type"] = "Online" + data["assetUrl"] = pdf_url + variant = vendor_variant_service.create(data) yield variant try: vendor_variant_service.delete(variant.id) except MPTAPIError as error: print(f"TEARDOWN - Unable to delete variant {variant.id}: {error.title}")If you adopt this, consider applying the same pattern to the async
created_variant_from_urlfixture for consistency. Based on relevant snippets fromtests/e2e/catalog/product/terms/variants/test_async_variants.pyandtests/e2e/catalog/product/terms/variants/conftest.py.tests/e2e/catalog/product/documents/test_async_document.py (1)
15-23: Async file‑based document fixture remains correct after logger removal
created_document_from_file_asyncstill setsdocumenttype, creates the document, and cleans it up reliably; dropping the logger arg just simplifies the fixture surface without behavior change. As a small future improvement, consider ensuringpdf_fdis closed (e.g., via a context-managed fixture) to avoid lingering open file handles during long test runs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
mpt_api_client/resources/catalog/pricing_policy_attachments.py(3 hunks)tests/e2e/catalog/items/test_async_item.py(1 hunks)tests/e2e/catalog/items/test_sync_item.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/conftest.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py(1 hunks)tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py(1 hunks)tests/e2e/catalog/pricing_policies/conftest.py(2 hunks)tests/e2e/catalog/pricing_policies/test_async_pricing_policies.py(1 hunks)tests/e2e/catalog/pricing_policies/test_sync_pricing_policies.py(0 hunks)tests/e2e/catalog/product/documents/test_async_document.py(2 hunks)tests/e2e/catalog/product/documents/test_sync_document.py(2 hunks)tests/e2e/catalog/product/item_groups/test_async_item_groups.py(1 hunks)tests/e2e/catalog/product/item_groups/test_sync_item_groups.py(1 hunks)tests/e2e/catalog/product/media/test_async_media.py(2 hunks)tests/e2e/catalog/product/media/test_sync_media.py(2 hunks)tests/e2e/catalog/product/parameter_groups/test_async_parameter_groups.py(1 hunks)tests/e2e/catalog/product/parameter_groups/test_sync_parameter_groups.py(1 hunks)tests/e2e/catalog/product/terms/test_sync_terms.py(2 hunks)tests/e2e/catalog/product/terms/variants/test_sync_variants.py(2 hunks)tests/e2e/catalog/product/test_async_product.py(1 hunks)tests/e2e/catalog/product/test_sync_product.py(1 hunks)tests/unit/resources/catalog/test_pricing_policy_attachments.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/catalog/pricing_policies/test_sync_pricing_policies.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/unit/resources/catalog/test_pricing_policy_attachments.py
- tests/e2e/catalog/product/item_groups/test_async_item_groups.py
- tests/e2e/catalog/product/media/test_async_media.py
- tests/e2e/catalog/product/terms/test_sync_terms.py
- tests/e2e/catalog/pricing_policies/attachments/test_async_attachment.py
- tests/e2e/catalog/pricing_policies/conftest.py
- tests/e2e/catalog/pricing_policies/attachments/conftest.py
- tests/e2e/catalog/product/test_async_product.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/catalog/product/parameter_groups/test_async_parameter_groups.pytests/e2e/catalog/product/documents/test_async_document.pytests/e2e/catalog/items/test_async_item.py
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/catalog/pricing_policies/test_async_pricing_policies.py
🧬 Code graph analysis (12)
tests/e2e/catalog/product/parameter_groups/test_sync_parameter_groups.py (2)
tests/e2e/conftest.py (2)
mpt_vendor(19-20)product_id(96-97)tests/e2e/catalog/product/parameter_groups/conftest.py (1)
parameter_group_data(5-10)
tests/e2e/catalog/product/parameter_groups/test_async_parameter_groups.py (2)
tests/e2e/conftest.py (2)
async_mpt_vendor(24-27)product_id(96-97)tests/e2e/catalog/product/parameter_groups/conftest.py (1)
parameter_group_data(5-10)
tests/e2e/catalog/product/terms/variants/test_sync_variants.py (3)
tests/e2e/catalog/product/terms/variants/conftest.py (1)
variant_data(5-12)tests/e2e/conftest.py (2)
pdf_fd(73-75)pdf_url(79-80)tests/e2e/catalog/product/terms/variants/test_async_variants.py (1)
created_variant_from_url(29-41)
tests/e2e/catalog/product/documents/test_async_document.py (3)
tests/unit/resources/catalog/test_mixins.py (1)
async_document_service(354-355)tests/e2e/catalog/product/documents/conftest.py (1)
document_data(10-16)tests/e2e/conftest.py (2)
pdf_fd(73-75)pdf_url(79-80)
tests/e2e/catalog/items/test_sync_item.py (2)
tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/catalog/items/conftest.py (1)
item_data(5-20)
tests/e2e/catalog/items/test_async_item.py (2)
tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/catalog/items/conftest.py (1)
item_data(5-20)
mpt_api_client/resources/catalog/pricing_policy_attachments.py (2)
mpt_api_client/http/mixins.py (2)
AsyncDownloadFileMixin(231-253)DownloadFileMixin(59-81)mpt_api_client/resources/catalog/mixins.py (2)
AsyncCreateFileMixin(84-110)CreateFileMixin(121-147)
tests/e2e/catalog/pricing_policies/test_async_pricing_policies.py (3)
tests/e2e/catalog/pricing_policies/test_sync_pricing_policies.py (1)
test_get_pricing_policy_by_id(21-24)tests/e2e/catalog/pricing_policies/conftest.py (1)
pricing_policy_id(41-42)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/catalog/product/media/test_sync_media.py (2)
tests/e2e/catalog/product/media/conftest.py (2)
media_data(5-14)test_media_file(18-19)tests/e2e/conftest.py (1)
jpg_url(84-85)
tests/e2e/catalog/product/documents/test_sync_document.py (2)
tests/e2e/catalog/product/documents/conftest.py (2)
vendor_document_service(20-21)document_data(10-16)tests/e2e/conftest.py (2)
pdf_fd(73-75)pdf_url(79-80)
tests/e2e/catalog/product/item_groups/test_sync_item_groups.py (2)
tests/e2e/conftest.py (2)
mpt_vendor(19-20)product_id(96-97)tests/e2e/catalog/product/item_groups/conftest.py (1)
item_group_data(5-15)
tests/e2e/catalog/product/test_sync_product.py (2)
tests/e2e/conftest.py (2)
mpt_vendor(19-20)logo_fd(106-108)tests/e2e/catalog/product/conftest.py (1)
product_data(5-6)
⏰ 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 (13)
tests/e2e/catalog/product/terms/variants/test_sync_variants.py (1)
15-21: Synchronouscreated_variantfixture + teardown pattern look goodThe create/yield/delete lifecycle and focused
MPTAPIErrorhandling are clear and match the async variant pattern; this keeps test setup/cleanup nicely encapsulated in the fixture.tests/e2e/catalog/product/parameter_groups/test_async_parameter_groups.py (1)
7-14: Fixture signature simplification looks goodDropping the unused
loggerparameter while keeping the async create/teardown logic unchanged is safe and consistent with how this fixture is used in the tests.tests/e2e/catalog/product/item_groups/test_sync_item_groups.py (1)
4-17: Import usage and fixture signature change are consistent
RQLQueryis correctly used intest_filter_item_groups, and removingloggerfromcreated_item_groupkeeps creation/teardown behavior intact without affecting call sites.tests/e2e/catalog/pricing_policies/test_async_pricing_policies.py (1)
38-41: Usingpricing_policy_idin async test is appropriateSwitching to the primitive
pricing_policy_idfixture makes this test mirror the sync variant and keeps the action (get) explicit while still exercising the async service.tests/e2e/catalog/product/test_sync_product.py (1)
7-16: Created product fixture refactor is safeRemoving
loggerfromcreated_productwithout changing its create/teardown logic preserves behavior and matches the broader fixture simplification in this PR.tests/e2e/catalog/product/parameter_groups/test_sync_parameter_groups.py (1)
7-14: Sync parameter group fixture change is straightforwardThe fixture still creates and deletes parameter groups correctly; dropping the
loggerargument simply streamlines its signature without impacting the tests.tests/e2e/catalog/product/media/test_sync_media.py (1)
14-31: Media fixtures signature updates are correctRemoving
loggerfromcreated_media_from_fileandcreated_media_from_urlkeeps creation and teardown behavior intact while simplifying fixture dependencies.mpt_api_client/resources/catalog/pricing_policy_attachments.py (1)
2-48: New create/download mixins and upload config are wired correctlySwitching pricing policy attachments to
CreateFileMixin/AsyncCreateFileMixinandDownloadFileMixin/AsyncDownloadFileMixin, with_upload_file_key = "file"and_upload_data_key = "attachment"inPricingPolicyAttachmentsServiceConfig, cleanly integrates with the shared mixin implementations and aligns with the new attachment tests.tests/e2e/catalog/items/test_sync_item.py (1)
9-17: Fixture signature simplification looks good
created_itemno longer depends on an unused logger and still encapsulates create/teardown correctly. Call sites in this file remain compatible.tests/e2e/catalog/items/test_async_item.py (1)
9-17: Async fixture cleanup maintains correct behaviorRemoving the logger arg from
async_created_itemis safe; the fixture still handles async create/teardown correctly. Using it intest_create_itemas a plain value is appropriate for pytest-asyncio’s async fixtures. Based on learnings, this pattern is fine.tests/e2e/catalog/product/documents/test_async_document.py (1)
25-33: Link‑based async document fixture cleanup is consistent
created_document_from_link_asynckeeps the same semantics (mutatedocument_data["url"], create, yield, delete with guarded teardown); removing the unused logger parameter simply aligns it with other fixtures.tests/e2e/catalog/product/documents/test_sync_document.py (2)
9-17: Synchronous file‑based document fixture simplification is safe
created_document_from_filestill encapsulates create/teardown correctly; dropping the logger argument reduces fixture coupling without changing behavior. Tests using this fixture remain valid.
20-28: URL‑based document fixture now matches the simplified fixture pattern
created_document_from_urllikewise keeps its semantics (seturl, create, delete on teardown) while removing an unused logger parameter, matching the pattern used elsewhere in the suite.
|
albertsola
deleted the
e2e/catalog/MPT-14900-pricing-policy-attachments
branch
Summary by CodeRabbit
New Features
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.