[MPT-16219] Added e2e tests for commerce orders #160
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdds sync/async render and quote methods to Orders services, updates e2e config and lint ignores, renames two attachment tests, adds E2E fixtures and comprehensive sync/async order E2E tests, and updates unit tests to use httpx.codes.OK and cover render/quote behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
robcsegal
force-pushed
the
MPT-16219-add-e-2-e-tests-for-commerce-orders
branch
from
385142b to
094ff1c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
55-59: Make this test async (syncdef+ async fixture can silently skip the create).As written, this can devolve into asserting a coroutine is non-null, rather than asserting a created attachment exists.
-def test_create_order_agreement_attachment(created_agreement_attachment): +async def test_create_order_agreement_attachment(created_agreement_attachment): result = created_agreement_attachment assert result is not None
🧹 Nitpick comments (4)
tests/unit/resources/commerce/test_orders.py (1)
240-254: Add route-hit assertions in async template/render tests for parity with sync tests.
Right now these tests only assert returned content; they won’t catch “method never called” regressions as directly as the sync variants do.async def test_async_template(async_orders_service): template_content = "# Order Template\n\nThis is a markdown template." with respx.mock: - respx.get("https://api.example.com/public/v1/commerce/orders/ORD-123/template").mock( + route = respx.get("https://api.example.com/public/v1/commerce/orders/ORD-123/template").mock( return_value=httpx.Response( status_code=httpx.codes.OK, headers={"content-type": "text/markdown"}, content=template_content, ) ) result = await async_orders_service.template("ORD-123") + assert route.called + assert route.call_count == 1 assert result == template_contentAlso applies to: 256-270
tests/e2e/commerce/order/test_sync_order.py (2)
27-33:test_list_ordersmay be env-dependent (can flake on empty tenants).
Consider depending oncreated_order(or creating one inside the test) so the assertion doesn’t rely on pre-existing data.
9-19: Consider adding teardown to avoid leaving e2e data behind.
Ayieldfixture that deletes the created order infinallywill reduce tenant pollution over time.tests/e2e/commerce/order/conftest.py (1)
10-55:freeze_timeon the fixture likely doesn’t freeze the time whenfactory()is called.
If you intended deterministic timestamps, move the freeze into the returned closure (or freeze around the create call in the tests). Otherwise, consider dropping freezegun here to avoid misleading determinism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/orders.py(2 hunks)pyproject.toml(1 hunks)tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/commerce/order/conftest.py(1 hunks)tests/e2e/commerce/order/test_async_order.py(1 hunks)tests/e2e/commerce/order/test_sync_order.py(1 hunks)tests/unit/resources/commerce/test_agreements.py(4 hunks)tests/unit/resources/commerce/test_assets.py(4 hunks)tests/unit/resources/commerce/test_orders.py(14 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
pyproject.toml
🧬 Code graph analysis (6)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py (2)
test_create_order_agreement_attachment(57-60)created_agreement_attachment(10-15)
tests/e2e/commerce/order/test_sync_order.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/commerce/order/test_async_order.py (3)
created_order(10-13)processed_order(17-18)test_get_order_by_id(21-24)tests/e2e/conftest.py (2)
mpt_client(43-44)mpt_vendor(19-20)tests/e2e/commerce/order/conftest.py (3)
order_factory(12-54)invalid_order_id(6-7)order_subscription_factory(58-72)mpt_api_client/resources/commerce/orders.py (14)
process(51-58)process(181-191)query(60-67)query(193-203)subscriptions(132-144)subscriptions(274-286)complete(69-76)complete(205-215)fail(78-85)fail(217-227)quote(120-130)quote(262-272)template(96-106)template(238-248)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/commerce/order/test_async_order.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (2)
async_mpt_client(48-51)async_mpt_vendor(24-27)tests/e2e/commerce/order/conftest.py (3)
order_factory(12-54)invalid_order_id(6-7)order_subscription_factory(58-72)mpt_api_client/resources/commerce/orders.py (20)
process(51-58)process(181-191)query(60-67)query(193-203)subscriptions(132-144)subscriptions(274-286)complete(69-76)complete(205-215)fail(78-85)fail(217-227)quote(120-130)quote(262-272)validate(42-49)validate(169-179)template(96-106)template(238-248)render(108-118)render(250-260)notify(87-94)notify(229-236)
tests/e2e/commerce/order/conftest.py (2)
tests/e2e/conftest.py (1)
licensee_id(142-143)tests/e2e/commerce/conftest.py (2)
commerce_product_id(10-11)commerce_item_id(20-21)
mpt_api_client/resources/commerce/orders.py (5)
mpt_api_client/resources/commerce/orders_asset.py (2)
render(31-45)render(56-70)mpt_api_client/resources/commerce/agreements.py (2)
render(47-57)render(94-104)mpt_api_client/resources/commerce/subscriptions.py (2)
render(40-50)render(75-85)mpt_api_client/http/async_service.py (2)
_resource_do_request(24-53)_resource_action(55-80)mpt_api_client/http/service.py (2)
_resource_do_request(24-53)_resource_action(55-80)
⏰ 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)
tests/unit/resources/commerce/test_agreements.py (1)
16-22: Status-code constant swap is fine; verifyhttpx.codes.OKfor pinned httpx.This is a clean readability win and keeps behavior the same. Since it touches an
httpxAPI surface, just ensurehttpx.codes.OKis present/compatible with your pinnedhttpx==0.28.*.Also applies to: 32-40, 53-61, 74-82
tests/unit/resources/commerce/test_assets.py (1)
22-28: Consistent use ofhttpx.codes.OKin mocks looks good (verify against pinned httpx).No behavioral change here; it just standardizes status codes. Please double-check
httpx.codes.OKis supported as expected underhttpx==0.28.*.Also applies to: 41-46, 61-66, 81-86
tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py (1)
57-61: Test rename is harmless and improves suite clarity.tests/e2e/commerce/conftest.py (1)
14-31: Fixtures are simple and consistent; OK as-is.(Optional) If you want clearer failures than
KeyError, you could wrap with a message, but this is fine for e2e.tests/unit/resources/commerce/test_orders.py (2)
26-36:quoteaction coverage looks consistent with existing resource-action tests.
Using the same paramized harness forquoteas validate/process/query/etc. is a good fit, and the “no data” variant should keep guarding the “no body sent” behavior.Also applies to: 60-70, 152-162, 186-196
44-48: Nice consistency: preferhttpx.codes.OKover raw200.Also applies to: 78-82, 99-104, 119-124, 138-143, 170-175, 204-209, 225-230, 244-249, 260-265
tests/e2e/commerce/order/test_async_order.py (1)
6-6: Same marker concern as sync file: verifyflakymarker is registered.tests/e2e/commerce/order/test_sync_order.py (1)
6-6: Thepytest.mark.flakymarker in the pytest configuration does not require registration—the repo does not use--strict-markers, and unregistered markers are allowed by default.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mpt_api_client/resources/commerce/orders.py (1)
108-118: Docstring forrender()still claims “markdown”; consider making it content-type agnostic.
This was flagged previously and remains unchanged; the client returnsresponse.textand the API may return HTML/markdown depending on endpoint behavior.- Returns: - Render template text in markdown format. + Returns: + Rendered template text.Also applies to: 250-260
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/orders.py(2 hunks)pyproject.toml(1 hunks)tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/commerce/order/conftest.py(1 hunks)tests/e2e/commerce/order/test_async_order.py(1 hunks)tests/e2e/commerce/order/test_sync_order.py(1 hunks)tests/unit/resources/commerce/test_agreements.py(4 hunks)tests/unit/resources/commerce/test_assets.py(4 hunks)tests/unit/resources/commerce/test_orders.py(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- e2e_config.test.json
- tests/e2e/commerce/order/conftest.py
- tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py
- pyproject.toml
- tests/e2e/commerce/order/test_sync_order.py
- tests/e2e/commerce/conftest.py
- tests/unit/resources/commerce/test_assets.py
- tests/e2e/commerce/order/test_async_order.py
🧰 Additional context used
🧬 Code graph analysis (3)
mpt_api_client/resources/commerce/orders.py (5)
mpt_api_client/resources/commerce/orders_asset.py (2)
render(31-45)render(56-70)mpt_api_client/resources/commerce/agreements.py (2)
render(47-57)render(94-104)mpt_api_client/resources/commerce/subscriptions.py (2)
render(40-50)render(75-85)mpt_api_client/http/async_service.py (2)
_resource_do_request(24-53)_resource_action(55-80)mpt_api_client/http/service.py (2)
_resource_do_request(24-53)_resource_action(55-80)
tests/unit/resources/commerce/test_orders.py (7)
tests/unit/resources/commerce/test_agreements.py (2)
test_render(49-67)test_async_render(70-88)tests/unit/resources/commerce/test_assets.py (2)
test_render(35-52)test_async_render(19-32)tests/unit/resources/commerce/test_orders_asset.py (2)
test_render(49-64)test_async_render(67-82)tests/unit/resources/commerce/test_subscriptions.py (2)
test_render(87-100)test_async_render(54-67)mpt_api_client/http/types.py (1)
Response(27-42)mpt_api_client/resources/commerce/orders.py (2)
render(108-118)render(250-260)mpt_api_client/resources/commerce/orders_asset.py (2)
render(31-45)render(56-70)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py (2)
test_create_order_agreement_attachment(57-60)created_agreement_attachment(10-15)
⏰ 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 (4)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
55-55: Verify the "order" terminology in the function name.The function is renamed to
test_create_order_agreement_attachment, but it's testing the creation of agreement attachments, not specifically order-related agreement attachments. Verify that this naming aligns with the intended test scope and doesn't cause confusion.mpt_api_client/resources/commerce/orders.py (1)
120-130:quote()implementation matches existing_resource_actionpatterns.
Looks consistent and well-covered by the updated unit tests.Also applies to: 262-273
tests/unit/resources/commerce/test_agreements.py (1)
17-21: Consistent switch tohttpx.codes.OKlooks good.
Improves readability and aligns with the rest of the suite.In httpx 0.28.0, is `httpx.codes.OK` the recommended/available way to specify status codes in `httpx.Response(status_code=...)`? If not, what is the preferred constant/API (e.g., `httpx.codes.HTTP_200_OK`, `httpx.codes.OK`, etc.)?Also applies to: 35-39, 56-60, 77-81
tests/unit/resources/commerce/test_orders.py (1)
26-58: Nice coverage addition forquote+render(sync/async), and status-code consistency is solid.No correctness issues spotted in the new/updated tests. The use of
httpx.codes.OKis idiomatic and stable in httpx 0.28.0, properly evaluating to the integer 200 and accepted byhttpx.Response(status_code=...).
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py
Show resolved
Hide resolved
robcsegal
force-pushed
the
MPT-16219-add-e-2-e-tests-for-commerce-orders
branch
from
094ff1c to
6b87970
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
55-58: Rename is fine; keep this test synchronous (noawaitneeded).
Given the body has noawait,defis the right choice even with an async fixture (avoids “unnecessary async” lint).tests/e2e/commerce/order/test_async_order.py (1)
134-141:notify()afterquote()likely targets the wrong order id—confirm API semantics.
Youquote(created_order.id)but thennotify(created_order.id, ...). If quote produces a distinct “quoted order” resource/id, you probably want to notify that id instead.async def test_notify_order(async_mpt_client, created_order, commerce_user_id): user_data = { "userId": commerce_user_id, "notifyMe": False, } - await async_mpt_client.commerce.orders.quote(created_order.id) - - await async_mpt_client.commerce.orders.notify(created_order.id, user_data) + quoted = await async_mpt_client.commerce.orders.quote(created_order.id) + await async_mpt_client.commerce.orders.notify(quoted.id, user_data)
🧹 Nitpick comments (1)
tests/e2e/commerce/order/test_async_order.py (1)
9-19: Fixtures are clear; consider cleanup if the environment accumulates E2E data.
created_ordercreates persistent server state; if the backend doesn’t auto-expire, consider a teardown (or a periodic cleanup job) to prevent long-term pollution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/orders.py(2 hunks)pyproject.toml(1 hunks)tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/commerce/order/conftest.py(1 hunks)tests/e2e/commerce/order/test_async_order.py(1 hunks)tests/e2e/commerce/order/test_sync_order.py(1 hunks)tests/unit/resources/commerce/test_agreements.py(4 hunks)tests/unit/resources/commerce/test_assets.py(4 hunks)tests/unit/resources/commerce/test_orders.py(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- e2e_config.test.json
- tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py
- mpt_api_client/resources/commerce/orders.py
- tests/e2e/commerce/order/test_sync_order.py
- tests/unit/resources/commerce/test_agreements.py
- tests/e2e/commerce/conftest.py
- tests/e2e/commerce/order/conftest.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
pyproject.toml
📚 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:
pyproject.tomltests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.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/agreement/attachment/test_async_agreement_attachment.pytests/unit/resources/commerce/test_assets.pytests/unit/resources/commerce/test_orders.pytests/e2e/commerce/order/test_async_order.py
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py
🧬 Code graph analysis (2)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py (2)
test_create_order_agreement_attachment(57-60)created_agreement_attachment(10-15)
tests/unit/resources/commerce/test_orders.py (3)
mpt_api_client/http/types.py (1)
Response(27-42)mpt_api_client/resources/commerce/orders.py (2)
render(108-118)render(250-260)mpt_api_client/resources/commerce/orders_asset.py (2)
render(31-45)render(56-70)
⏰ 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)
tests/unit/resources/commerce/test_assets.py (1)
24-24: LGTM! Improved readability with httpx status codes.Replacing magic number
200withhttpx.codes.OKimproves code clarity and aligns with httpx best practices.Also applies to: 42-42, 62-62, 82-82
tests/unit/resources/commerce/test_orders.py (3)
34-34: LGTM! Quote action test coverage added.The new "quote" action is properly tested across all scenarios (sync/async, with/without data), following the established pattern for other order actions.
Also applies to: 68-68, 160-160, 194-194
45-45: LGTM! Consistent use of httpx status codes.Replacing
200withhttpx.codes.OKthroughout improves readability and maintains consistency with the refactoring in test_assets.py.Also applies to: 79-79, 100-100, 120-120, 139-139, 171-171, 205-205, 226-226, 245-245, 261-261
133-149: LGTM! Render functionality properly tested.Both sync and async render tests are well-structured and follow the established pattern of the template tests. They correctly verify that the render endpoint returns text content.
Also applies to: 256-269
pyproject.toml (2)
116-117: Per-file ignores formpt_api_client/mpt_client.py/mpt_api_client/resources/*look consistent with WPS usage.
No concerns as long as these are addressing known WPS noise (and not masking regressions).
124-130: E2E/unit/seed WPS per-file-ignores are reasonable—double-check glob intent and coverage.
These new patterns are broad (by design). Just confirm they match the intended file layout (flake8’s path matching can be finicky across tooling).Also applies to: 135-136, 143-143
tests/e2e/commerce/order/test_async_order.py (2)
21-51: Query/list/filter coverage looks good.
TheRQLQueryfilter +.select()+ async iteration exercise a valuable slice of the client surface.
53-57: Good call keepingtest_create_ordersynchronous.
This avoids “unnecessary async” lint while still benefiting from async fixture resolution.
|
Added e2e tests for commerce orders
https://softwareone.atlassian.net/browse/MPT-16219
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.