[MPT-14904] Created e2e tests for commerce agreements #141
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdded commerce identifiers to e2e config, implemented sync and async AgreementsService.render methods, reorganized/added pytest fixtures, and introduced new end-to-end and unit tests for commerce agreements (CRUD, filter, render, template, and error cases). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
tests/e2e/commerce/agreement/conftest.py (1)
15-60: LGTM!The agreement_factory fixture is well-designed with:
- Deterministic time behavior via @freeze_time
- Proper use of get_iso_dt_str for ISO datetime formatting
- Comprehensive agreement payload covering all required fields
- Flexible parameters for customization
Remove unused noqa directives.
Lines 16 and 25 have
noqadirectives for wemake-python-styleguide rules (WPS211, WPS430) that Ruff flags as unused. If wemake-python-styleguide is not part of your linting pipeline, consider removing them.@pytest.fixture -def agreement_factory( # noqa: WPS211 +def agreement_factory( account_id, seller_id, buyer_id, licensee_id, commerce_product_id, authorization_id, ): @freeze_time("2025-11-14T09:00:00.000Z") - def _agreement_factory( # noqa: WPS430 + def _agreement_factory( name: str = "E2E Created Agreement", client_external_id: str = "test-client-external-id", vendor_external_id: str = "test-vendor-external-id", ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements.py(2 hunks)mpt_api_client/utils.py(1 hunks)pyproject.toml(2 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/accounts/licensees/conftest.py(0 hunks)tests/e2e/commerce/agreement/conftest.py(1 hunks)tests/e2e/commerce/agreement/test_async_agreement.py(1 hunks)tests/e2e/commerce/agreement/test_sync_agreement.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/commerce/test_agreement.py(1 hunks)tests/seed/commerce/test_commerce.py(1 hunks)tests/seed/test_seed_api.py(2 hunks)tests/unit/resources/commerce/test_agreements.py(1 hunks)tests/unit/test_utils.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/licensees/conftest.py
- tests/e2e/accounts/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📚 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/commerce/conftest.py
📚 Learning: 2025-12-01T10:48:52.572Z
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.572Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
pyproject.toml
🧬 Code graph analysis (5)
tests/unit/resources/commerce/test_agreements.py (1)
mpt_api_client/resources/commerce/agreements.py (2)
render(47-57)render(94-104)
mpt_api_client/resources/commerce/agreements.py (3)
mpt_api_client/resources/commerce/subscriptions.py (2)
render(40-50)render(75-85)mpt_api_client/http/async_service.py (1)
_resource_do_request(24-53)mpt_api_client/http/service.py (1)
_resource_do_request(24-53)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/unit/test_utils.py (1)
mpt_api_client/utils.py (1)
get_iso_dt_str(4-6)
tests/e2e/commerce/agreement/conftest.py (3)
mpt_api_client/utils.py (1)
get_iso_dt_str(4-6)tests/e2e/conftest.py (5)
account_id(122-123)seller_id(127-128)buyer_id(132-133)licensee_id(137-138)authorization_id(142-143)tests/e2e/commerce/conftest.py (1)
commerce_product_id(10-11)
🪛 Ruff (0.14.7)
tests/seed/commerce/test_agreement.py
60-60: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
tests/e2e/commerce/agreement/conftest.py
16-16: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
25-25: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (22)
pyproject.toml (2)
51-51: LGTM!Adding
types-python-dateutilfor type checking support is appropriate given the new datetime utility functions.
123-123: LGTM!The per-file ignores for the new utils module are appropriate.
tests/seed/commerce/test_commerce.py (1)
1-14: LGTM!The test properly mocks the async dependency and verifies the expected call. The test structure follows best practices for async testing.
tests/seed/test_seed_api.py (1)
26-26: LGTM!The additions properly extend the test to cover the new
seed_commercefunction, following the established pattern.Also applies to: 38-38
tests/unit/test_utils.py (1)
1-14: LGTM!The test properly validates the ISO datetime string conversion using frozen time. The test is clear and focused.
e2e_config.test.json (1)
14-20: LGTM!The new commerce-related test fixture IDs are properly structured and consistent with the existing configuration format.
tests/unit/resources/commerce/test_agreements.py (2)
49-67: LGTM!The test properly validates the synchronous render functionality, following the established pattern from the template tests.
70-88: LGTM!The async test properly validates the asynchronous render functionality with appropriate mocking and assertions.
tests/e2e/commerce/agreement/test_sync_agreement.py (4)
9-15: LGTM!The fixture properly creates a test agreement using the factory pattern for reproducible test data.
18-47: LGTM!The read and filter tests provide good coverage, including error handling for not-found scenarios and RQL query filtering.
50-62: LGTM!The create and update tests properly validate the CRUD operations on agreements.
65-73: LGTM!The render and template tests validate the new agreement rendering functionality.
tests/e2e/commerce/conftest.py (1)
1-11: LGTM!The fixtures follow the established pattern for configuration accessors and align well with the commerce test structure.
tests/e2e/conftest.py (1)
121-143: LGTM!The new fixtures follow the established pattern and consolidate configuration access from child modules to the parent conftest, reducing duplication across the test suite.
tests/e2e/commerce/agreement/test_async_agreement.py (4)
9-15: LGTM!The fixture properly utilizes the agreement_factory to create test data for subsequent tests.
18-35: LGTM!Basic CRUD operations and error handling tests are well-structured.
51-77: LGTM!The create, update, render, and template tests appropriately validate the agreement operations.
37-48: Hardcoded agreement name must match seeded test data.Line 42 filters by a hardcoded name
"E2E Seeded For Commerce for Test Api Client Client"that must exactly match the seeded agreement (AGR-9850-2169-6098). This name is not generated by any fixture or factory and depends entirely on the test environment's seeded data. If the agreement's actual name differs, the filter will return no results and the test will fail silently.tests/seed/commerce/test_agreement.py (1)
34-57: LGTM!The test functions for get_agreement, get_agreement_without_id, and set_agreement properly validate the seed module's behavior with appropriate mocking.
mpt_api_client/resources/commerce/agreements.py (2)
47-57: LGTM!The synchronous render method follows the established pattern from the template method and aligns with similar implementations in subscriptions.py.
94-104: LGTM!The asynchronous render method correctly mirrors the synchronous version and maintains consistency with the template method pattern.
tests/e2e/commerce/agreement/conftest.py (1)
10-12: LGTM!The invalid_agreement_id fixture provides a deterministic invalid ID for testing error scenarios.
robcsegal
force-pushed
the
MPT-14904-add-e-2-e-tests-for-commerce-agreements
branch
from
f783387 to
f30c27d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/e2e/commerce/agreement/conftest.py (1)
10-60: Deterministicagreement_factorylooks good; consider dropping unusednoqacommentsThe factory setup (frozen time, UTC-aware
datetime.now,relativedelta, andget_iso_dt_str) gives deterministic, well-structured agreement payloads, which is ideal for e2e tests.Ruff is flagging the
# noqa: WPS211on the fixture and# noqa: WPS430on the inner factory as unused/unknown. If wemake codes are no longer enforced in this file, you can safely remove thosenoqadirectives to keep linters quiet; otherwise, consider adjusting the Ruff config instead. Based on static analysis hints.mpt_api_client/utils.py (1)
4-7: UTC normalization implementation is correct; clarify expectation for tz-aware inputsThe function now explicitly normalizes to UTC via
astimezone(dt.UTC)and emits a millisecond-precision ISO string with"Z", which aligns with the intended behavior and the new tests.If callers might ever pass naive datetimes, consider either documenting that
dt_objmust be timezone-aware (UTC or convertible to UTC) or adding a small guard/normalization for naive inputs; otherwise this will raise at runtime.tests/e2e/commerce/agreement/test_sync_agreement.py (1)
9-74: E2E agreement coverage is solid; consider stronger create/update assertionsThis suite gives broad coverage over agreements (get, list, 404, filtering with
RQLQuery, create, update, render, template) and ties nicely into the seeded/fixture data.To make the tests more robust, you might consider:
- In
test_create_agreement, asserting on specific fields (e.g.,result.nameorresult.id) rather than onlyis not None.- In
test_update_agreement, asserting thatresult.name == updated_name(or re-fetching and checking) to verify the update actually took effect, not just that a response was returned.These are incremental improvements; the current tests are already valuable.
mpt_api_client/resources/commerce/agreements.py (1)
47-57: Render methods correctly delegate to_resource_do_request; adjust docstringsThe sync and async
rendermethods correctly use_resource_do_requestwithaction="render"and returnresponse.text, consistent with the rest of the HTTP service layer and the new tests.Minor nit: the docstrings still say “Renders the template for the given Agreement id.” — you might tweak the wording to distinguish these from the
templatemethods (e.g., “Renders the Agreement for the given id.”) to avoid confusion.Also applies to: 94-104
tests/e2e/commerce/agreement/test_async_agreement.py (1)
51-54: Make the test function async for consistency.This test is synchronous while all other tests in the file are asynchronous, creating inconsistency in the test suite.
Apply this diff to align with other tests:
-def test_create_agreement(created_agreement): +async def test_create_agreement(created_agreement): result = created_agreement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements.py(2 hunks)mpt_api_client/utils.py(1 hunks)pyproject.toml(2 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/accounts/licensees/conftest.py(0 hunks)tests/e2e/commerce/agreement/conftest.py(1 hunks)tests/e2e/commerce/agreement/test_async_agreement.py(1 hunks)tests/e2e/commerce/agreement/test_sync_agreement.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/test_seed_api.py(2 hunks)tests/unit/resources/commerce/test_agreements.py(1 hunks)tests/unit/test_utils.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/licensees/conftest.py
- tests/e2e/accounts/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- tests/seed/test_seed_api.py
- e2e_config.test.json
🧰 Additional context used
🧠 Learnings (1)
📚 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/commerce/conftest.pytests/e2e/conftest.py
🧬 Code graph analysis (5)
tests/unit/test_utils.py (1)
mpt_api_client/utils.py (1)
get_iso_dt_str(4-7)
tests/unit/resources/commerce/test_agreements.py (2)
mpt_api_client/resources/commerce/agreements.py (2)
render(47-57)render(94-104)mpt_api_client/http/types.py (1)
Response(27-42)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
mpt_api_client/resources/commerce/agreements.py (3)
mpt_api_client/resources/commerce/subscriptions.py (2)
render(40-50)render(75-85)mpt_api_client/http/async_service.py (1)
_resource_do_request(24-53)mpt_api_client/http/service.py (1)
_resource_do_request(24-53)
tests/e2e/commerce/agreement/conftest.py (3)
mpt_api_client/utils.py (1)
get_iso_dt_str(4-7)tests/e2e/conftest.py (5)
account_id(122-123)seller_id(127-128)buyer_id(132-133)licensee_id(137-138)authorization_id(142-143)tests/e2e/commerce/conftest.py (1)
commerce_product_id(10-11)
🪛 Ruff (0.14.7)
tests/e2e/commerce/agreement/conftest.py
16-16: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
25-25: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
tests/e2e/conftest.py (1)
121-143: Centralized ID fixtures are consistent and usefulThe new account/seller/buyer/licensee/authorization ID fixtures follow the existing
e2e_configpattern and nicely centralize shared IDs for reuse across e2e suites. No changes needed. Based on learnings, not suggesting changes to any childconftest.pyfiles that may still define similar fixtures.tests/unit/test_utils.py (1)
8-14: Unit test cleanly pins ISO8601 formatting behaviorThe test under frozen UTC time verifies both the normalization and the exact ISO 8601 representation, tightly covering
get_iso_dt_str’s contract.tests/unit/resources/commerce/test_agreements.py (1)
49-88: Render tests correctly mirror template behaviorThe new sync and async
rendertests closely follow the existing template tests, verifying the/renderURL, single call to the mocked route, and passthrough of the response body. This gives good coverage for the new service methods.tests/e2e/commerce/conftest.py (1)
4-11: Commerce ID fixtures align with existing e2e config patternBoth
agreement_idandcommerce_product_idfixtures match the establishede2e_config[...]accessor style and keep commerce IDs nicely scoped for e2e tests.
robcsegal
force-pushed
the
MPT-14904-add-e-2-e-tests-for-commerce-agreements
branch
from
f30c27d to
1d56cb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/commerce/agreement/test_sync_agreement.py (1)
18-74: Consider adding more specific assertions to strengthen test validation.While the current
assert result is not Nonechecks confirm that operations return data, they don't validate the correctness of the returned values. For example:
- In
test_get_agreement_by_id, verify thatresult.id == agreement_id- In
test_update_agreement, verify thatresult.name == updated_name- In
test_filter_agreements, verify that the filtered result contains the expected agreementThese additional checks would make the tests more robust and catch potential data inconsistencies.
tests/e2e/commerce/agreement/test_async_agreement.py (2)
50-53: Consider making this test async for consistency.While pytest-asyncio will handle the async
created_agreementfixture correctly, making this testasync defwould be more consistent with the other tests in this file.Apply this diff:
-def test_create_agreement(created_agreement): +async def test_create_agreement(created_agreement): result = created_agreement
18-76: Consider adding more specific assertions to strengthen test validation.Similar to the sync version, the current
assert result is not Nonechecks are minimal. Consider adding more specific validations such as:
- Verifying IDs match expected values
- Checking updated fields after updates
- Validating filtered results contain expected data
This would make the tests more robust and catch potential data inconsistencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements.py(2 hunks)mpt_api_client/utils.py(1 hunks)pyproject.toml(2 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/accounts/licensees/conftest.py(0 hunks)tests/e2e/commerce/agreement/conftest.py(1 hunks)tests/e2e/commerce/agreement/test_async_agreement.py(1 hunks)tests/e2e/commerce/agreement/test_sync_agreement.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/test_seed_api.py(2 hunks)tests/unit/resources/commerce/test_agreements.py(1 hunks)tests/unit/test_utils.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/conftest.py
- tests/e2e/accounts/licensees/conftest.py
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e_config.test.json
- mpt_api_client/utils.py
- mpt_api_client/resources/commerce/agreements.py
- tests/unit/resources/commerce/test_agreements.py
- tests/seed/test_seed_api.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-01T10:48:52.572Z
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.572Z
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.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/commerce/agreement/conftest.py
🧬 Code graph analysis (5)
tests/unit/test_utils.py (1)
mpt_api_client/utils.py (1)
get_iso_dt_str(4-7)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/commerce/agreement/test_async_agreement.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/commerce/agreement/conftest.py (1)
invalid_agreement_id(11-12)mpt_api_client/models/model.py (1)
id(119-121)mpt_api_client/resources/commerce/agreements.py (4)
render(47-57)render(94-104)template(35-45)template(82-92)
tests/e2e/commerce/agreement/test_sync_agreement.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
mpt_ops(31-32)tests/e2e/commerce/conftest.py (1)
agreement_id(5-6)
tests/e2e/commerce/agreement/conftest.py (3)
mpt_api_client/utils.py (1)
get_iso_dt_str(4-7)tests/e2e/conftest.py (5)
account_id(122-123)seller_id(127-128)buyer_id(132-133)licensee_id(137-138)authorization_id(142-143)tests/e2e/commerce/conftest.py (1)
commerce_product_id(10-11)
🪛 Ruff (0.14.7)
tests/e2e/commerce/agreement/conftest.py
16-16: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
25-25: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
pyproject.toml (1)
51-51: LGTM! Dependency and linter configuration support the new utility module.The addition of
types-python-dateutiland the per-file linter ignores appropriately support the newmpt_api_client/utils.pymodule introduced in this PR.Also applies to: 123-123
tests/unit/test_utils.py (1)
8-14: LGTM! Clean and focused unit test.The test correctly validates the ISO datetime conversion with frozen time for deterministic results.
tests/e2e/conftest.py (1)
121-143: LGTM! Centralized config fixtures for commerce tests.The new fixtures provide consistent access to shared account and commerce identifiers from the e2e configuration.
tests/e2e/commerce/conftest.py (1)
4-11: LGTM! Commerce-specific configuration fixtures.The fixtures provide convenient access to commerce test identifiers from the e2e configuration.
tests/e2e/commerce/agreement/test_sync_agreement.py (1)
9-16: LGTM! Created agreement fixture follows the factory pattern.The fixture uses the agreement_factory to generate test data and creates the agreement via the API.
tests/e2e/commerce/agreement/conftest.py (2)
10-12: LGTM! Simple fixture for testing error cases.The hardcoded invalid ID enables consistent 404 testing across agreement tests.
15-60: LGTM! Well-designed agreement factory with time determinism.The factory fixture provides flexible agreement data generation with frozen time for reproducible test data. The noqa directives are valid for the wemake-python-styleguide linter used in this project, despite Ruff flagging them as unused.
Based on learnings, the wemake-python-styleguide linter is used in this repository, so the noqa directives for WPS rules should not be flagged as unused.
tests/e2e/commerce/agreement/test_async_agreement.py (2)
9-16: LGTM! Async created agreement fixture follows the factory pattern.The fixture correctly uses
awaitto create the agreement asynchronously.
37-47: LGTM! List comprehension correctly fixed.The variable reference issue from the previous review has been resolved. The comprehension now correctly appends
filtered_agreement(the iteration variable) rather thanfiltered_agreements(the query object).
robcsegal
changed the title
robcsegal
force-pushed
the
MPT-14904-add-e-2-e-tests-for-commerce-agreements
branch
from
1d56cb9 to
b36c487
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/commerce/agreement/conftest.py (1)
20-20: Remove unused# noqadirectives flagged by RuffRuff is reporting the
# noqa: WPS211and# noqa: WPS430comments as unused/unknown; if wemake‑python‑styleguide isn’t active in this project anymore, it’s better to drop these directives to keep the tests clean and avoid tooling noise.Also applies to: 29-29
tests/e2e/commerce/agreement/test_async_agreement.py (1)
1-76: Async agreement e2e coverage is solid; consider optional cleanup/assertion tweaksThese tests exercise the main async flows (CRUD, filtering, render, template, and 404 handling) using the shared fixtures, and the fixed filter comprehension now looks correct. If the target environment is long‑lived, you might optionally add teardown/cleanup for
created_agreement(or reuse a seeded agreement) and/or assert on key fields (likeidorname) rather than just non‑Noneto make the checks a bit stronger—but the current structure is already reasonable for e2e.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements.py(2 hunks)pyproject.toml(2 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/accounts/licensees/conftest.py(0 hunks)tests/e2e/commerce/agreement/conftest.py(1 hunks)tests/e2e/commerce/agreement/test_async_agreement.py(1 hunks)tests/e2e/commerce/agreement/test_sync_agreement.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/test_seed_api.py(2 hunks)tests/unit/resources/commerce/test_agreements.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/licensees/conftest.py
- tests/e2e/accounts/conftest.py
🚧 Files skipped from review as they are similar to previous changes (5)
- pyproject.toml
- tests/seed/test_seed_api.py
- e2e_config.test.json
- tests/unit/resources/commerce/test_agreements.py
- tests/e2e/commerce/agreement/test_sync_agreement.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/conftest.pytests/e2e/commerce/agreement/conftest.py
🧬 Code graph analysis (3)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
mpt_api_client/resources/commerce/agreements.py (4)
mpt_api_client/resources/commerce/subscriptions.py (2)
render(40-50)render(75-85)tests/e2e/commerce/conftest.py (1)
agreement_id(5-6)mpt_api_client/http/async_service.py (1)
_resource_do_request(24-53)mpt_api_client/http/service.py (1)
_resource_do_request(24-53)
tests/e2e/commerce/agreement/conftest.py (2)
tests/e2e/conftest.py (5)
account_id(122-123)seller_id(127-128)buyer_id(132-133)licensee_id(137-138)authorization_id(142-143)tests/e2e/commerce/conftest.py (1)
commerce_product_id(10-11)
🪛 Ruff (0.14.7)
tests/e2e/commerce/agreement/conftest.py
20-20: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
29-29: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
tests/e2e/conftest.py (1)
121-143: Centralized account/commerce ID fixtures look goodThe new ID fixtures follow the existing
e2e_configpattern, keep key names explicit, and provide a clean shared source for commerce/account identifiers. No issues from my side.tests/e2e/commerce/conftest.py (1)
1-11: Simple commerce ID fixtures are consistent and clear
agreement_idandcommerce_product_idmirror the existing fixture style and keep config access in one place; implementation is fine as-is.mpt_api_client/resources/commerce/agreements.py (1)
47-57: Agreementrendermethods correctly mirror existing patternsSync and async
renderboth delegate to_resource_do_request(..., action="render")and returnresponse.text, consistent with the subscriptions service and the existingtemplatemethods. This slots cleanly into the HTTP service abstraction.Also applies to: 94-104
tests/e2e/commerce/agreement/conftest.py (1)
8-62: Agreement factory and datetime handling are well-structured
agreement_factoryproduces a complete, deterministic payload (IDs, value block, dates, external IDs) usingfreeze_timeplusdatetime.now()and theget_iso_dt_strhelper, which is great for stable e2e tests. One small thing to double‑check:dt.UTCis only available in Python 3.11+, so please confirm the project’s minimum supported Python version; if 3.10 or earlier is still in scope, you’d need to switch todt.timezone.utcinstead.
robcsegal
force-pushed
the
MPT-14904-add-e-2-e-tests-for-commerce-agreements
branch
2 times, most recently
from
ec344b2 to
47e44a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/e2e/commerce/agreement/conftest.py (1)
20-24: Type hints should be removed per project conventions.
🧹 Nitpick comments (2)
tests/e2e/commerce/agreement/conftest.py (1)
11-11: Remove unusednoqadirective.Per the static analysis hint, the
noqa: WPS211directive is flagged as unused since Ruff doesn't recognize WPS-prefixed rules. If this project uses wemake-python-styleguide in addition to Ruff, verify whether this suppression is still needed; otherwise, remove it.@pytest.fixture -def agreement_factory( # noqa: WPS211 +def agreement_factory( account_id,tests/e2e/commerce/agreement/test_async_agreement.py (1)
52-55: Sync test function depends on async fixture.
test_create_agreementis not an async function but consumescreated_agreement, which is an async fixture. This works with pytest-asyncio's fixture resolution, but consider making this test async for consistency with the rest of the module.-def test_create_agreement(created_agreement): +async def test_create_agreement(created_agreement): result = created_agreement assert result is not None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements.py(2 hunks)pyproject.toml(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/accounts/licensees/conftest.py(0 hunks)tests/e2e/commerce/agreement/conftest.py(1 hunks)tests/e2e/commerce/agreement/test_async_agreement.py(1 hunks)tests/e2e/commerce/agreement/test_sync_agreement.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/test_seed_api.py(2 hunks)tests/unit/resources/commerce/test_agreements.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/conftest.py
- tests/e2e/accounts/licensees/conftest.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/e2e/conftest.py
- pyproject.toml
- tests/seed/test_seed_api.py
- e2e_config.test.json
- tests/unit/resources/commerce/test_agreements.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/agreement/conftest.py
🧬 Code graph analysis (3)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
mpt_api_client/resources/commerce/agreements.py (4)
mpt_api_client/resources/commerce/subscriptions.py (2)
render(40-50)render(75-85)mpt_api_client/http/service.py (1)
_resource_do_request(24-53)mpt_api_client/http/async_service.py (1)
_resource_do_request(24-53)mpt_api_client/http/types.py (1)
text(36-38)
tests/e2e/commerce/agreement/test_async_agreement.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
async_mpt_ops(36-39)tests/e2e/commerce/agreement/conftest.py (2)
agreement_factory(11-51)invalid_agreement_id(6-7)mpt_api_client/models/model.py (1)
id(119-121)mpt_api_client/resources/commerce/agreements.py (4)
render(47-57)render(94-104)template(35-45)template(82-92)
🪛 Ruff (0.14.7)
tests/e2e/commerce/agreement/conftest.py
11-11: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
mpt_api_client/resources/commerce/agreements.py (2)
47-57: LGTM! Clean implementation following established patterns.The synchronous
render()method correctly follows the same pattern as the existingtemplate()method and aligns with therender()implementation insubscriptions.py. The use of_resource_do_requestwith action="render" and returningresponse.textis idiomatic and consistent with the codebase conventions.
94-104: LGTM! Async implementation mirrors sync version correctly.The asynchronous
render()method correctly mirrors the synchronous version with proper use ofasync/await. It follows the same pattern as the existing asynctemplate()method and aligns with the asyncrender()implementation insubscriptions.py.tests/e2e/commerce/agreement/conftest.py (2)
5-7: LGTM!The
invalid_agreement_idfixture provides a clear, valid-format-but-nonexistent ID for testing 404 scenarios.
25-49: LGTM!The agreement payload structure is comprehensive and correctly uses the injected fixture IDs. The hardcoded dates align with the
freeze_timedecorator.tests/e2e/commerce/agreement/test_sync_agreement.py (4)
9-15: LGTM!The fixture correctly creates an agreement for tests that need a pre-existing resource.
18-29: LGTM!Basic retrieval and pagination tests are appropriately structured.
39-49: LGTM!The filter test demonstrates proper RQL query chaining with field selection.
52-76: LGTM!Create, update, render, and template tests are well-structured and cover the key agreement operations.
tests/e2e/commerce/conftest.py (1)
4-11: LGTM!Both fixtures correctly expose commerce-related IDs from the e2e configuration, following the established pattern for config-based fixtures.
tests/e2e/commerce/agreement/test_async_agreement.py (4)
9-15: LGTM!The async fixture correctly creates an agreement for dependent tests.
18-29: LGTM!Async retrieval and pagination tests are properly structured.
39-49: LGTM!The async list comprehension correctly iterates over the filtered results.
58-78: LGTM!Update, render, and template async tests are well-structured.
robcsegal
force-pushed
the
MPT-14904-add-e-2-e-tests-for-commerce-agreements
branch
from
47e44a5 to
c369447
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mpt_api_client/resources/commerce/agreements.py (1)
47-57:renderimplementations are correct; consider tightening docstringsThe sync and async
rendermethods correctly delegate to_resource_do_request(..., action="render")and returnresponse.text, consistent withtemplateand other services.
You might optionally update the first docstring line to say something like “Renders the Agreement for the given id.” to better distinguish it fromtemplate.Also applies to: 94-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements.py(2 hunks)pyproject.toml(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/accounts/licensees/conftest.py(0 hunks)tests/e2e/commerce/agreement/conftest.py(1 hunks)tests/e2e/commerce/agreement/test_async_agreement.py(1 hunks)tests/e2e/commerce/agreement/test_sync_agreement.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/test_seed_api.py(2 hunks)tests/unit/resources/commerce/test_agreements.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/licensees/conftest.py
- tests/e2e/accounts/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- tests/seed/test_seed_api.py
- tests/e2e/commerce/agreement/test_async_agreement.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/agreement/conftest.py
🧬 Code graph analysis (4)
mpt_api_client/resources/commerce/agreements.py (4)
mpt_api_client/resources/commerce/subscriptions.py (2)
render(40-50)render(75-85)mpt_api_client/http/async_service.py (1)
_resource_do_request(24-53)mpt_api_client/http/service.py (1)
_resource_do_request(24-53)mpt_api_client/http/types.py (1)
text(36-38)
tests/unit/resources/commerce/test_agreements.py (3)
tests/unit/conftest.py (2)
http_client(17-18)async_http_client(22-23)mpt_api_client/resources/commerce/agreements.py (2)
render(47-57)render(94-104)mpt_api_client/http/types.py (1)
Response(27-42)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/commerce/agreement/conftest.py (2)
tests/e2e/conftest.py (5)
account_id(122-123)seller_id(127-128)buyer_id(132-133)licensee_id(137-138)authorization_id(142-143)tests/e2e/commerce/conftest.py (1)
commerce_product_id(10-11)
🪛 Ruff (0.14.7)
tests/e2e/commerce/agreement/test_sync_agreement.py
1-1: Unused noqa directive (unknown: AAA04, AAA05)
Remove unused noqa directive
(RUF100)
tests/e2e/commerce/agreement/conftest.py
11-11: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
tests/e2e/conftest.py (1)
121-143: New account/commerce ID fixtures are consistent with existing config-driven patternThe added fixtures cleanly expose shared IDs from
e2e_configand follow the same style asuser_id/product_id; no changes needed.e2e_config.test.json (1)
14-20: Commerce IDs added to config align with new fixtures/testsThe new
commerce.*entries match the keys used in the e2e fixtures and tests and follow the existing ID format; looks good.tests/unit/resources/commerce/test_agreements.py (1)
49-88: Render unit tests provide solid coverage and mirror template testsThe sync and async
rendertests correctly mock the/renderendpoint, assert the route is called once, and verify the returned body matches; this aligns well with the existingtemplatetests.tests/e2e/commerce/conftest.py (1)
1-11: Commerce e2e fixtures are simple and consistent
agreement_idandcommerce_product_idcleanly expose the newcommerce.*config entries for tests; no changes needed.
robcsegal
force-pushed
the
MPT-14904-add-e-2-e-tests-for-commerce-agreements
branch
from
c369447 to
a6e063b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/commerce/agreement/conftest.py (1)
11-11: Consider removing thenoqa: WPS211directive.Ruff reports this directive as unused/unknown, and a past reviewer suggested removing it. However, if the wemake-python-styleguide linter enforces WPS211 (too many arguments) for this 6-parameter function, the directive should be retained.
If wemake doesn't complain after removal, apply this diff:
-def agreement_factory( # noqa: WPS211 +def agreement_factory( account_id, seller_id,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements.py(2 hunks)pyproject.toml(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/accounts/licensees/conftest.py(0 hunks)tests/e2e/commerce/agreement/conftest.py(1 hunks)tests/e2e/commerce/agreement/test_async_agreement.py(1 hunks)tests/e2e/commerce/agreement/test_sync_agreement.py(1 hunks)tests/e2e/commerce/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/test_seed_api.py(2 hunks)tests/unit/resources/commerce/test_agreements.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/conftest.py
- tests/e2e/accounts/licensees/conftest.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/unit/resources/commerce/test_agreements.py
- pyproject.toml
- mpt_api_client/resources/commerce/agreements.py
- tests/seed/test_seed_api.py
- tests/e2e/commerce/agreement/test_async_agreement.py
🧰 Additional context used
🧠 Learnings (2)
📚 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/agreement/test_sync_agreement.pytests/e2e/commerce/agreement/conftest.py
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/commerce/agreement/conftest.py
🧬 Code graph analysis (2)
tests/e2e/commerce/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/commerce/agreement/test_sync_agreement.py (3)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/commerce/agreement/conftest.py (2)
agreement_factory(11-51)invalid_agreement_id(6-7)mpt_api_client/resources/commerce/agreements.py (4)
render(47-57)render(94-104)template(35-45)template(82-92)
🪛 Ruff (0.14.7)
tests/e2e/commerce/agreement/conftest.py
11-11: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (14)
tests/e2e/commerce/conftest.py (1)
1-11: LGTM!The fixtures follow the established pattern for config-backed fixtures in this repository.
tests/e2e/conftest.py (1)
121-143: LGTM!The new fixtures follow the established pattern and support the PR's goal of centralizing shared identifiers.
tests/e2e/commerce/agreement/test_sync_agreement.py (9)
9-15: LGTM!The fixture correctly creates an agreement for testing purposes.
18-21: LGTM!The test correctly verifies that an agreement can be retrieved by ID.
24-29: LGTM!The test correctly verifies agreement listing with pagination.
32-34: LGTM!The test correctly verifies that attempting to get a non-existent agreement raises
MPTAPIErrorwith a 404 status.
37-47: LGTM!The test correctly verifies RQL filtering with field selection.
50-53: LGTM!The test correctly verifies agreement creation through the fixture.
56-62: LGTM!The test correctly verifies agreement updates by modifying the name field.
65-68: LGTM!The test correctly verifies the new agreement render functionality.
71-74: LGTM!The test correctly verifies the agreement template retrieval functionality.
e2e_config.test.json (1)
14-20: LGTM!The new commerce identifiers are properly formatted and follow the established naming conventions.
tests/e2e/commerce/agreement/conftest.py (2)
5-7: LGTM!The fixture provides a consistent invalid ID for testing 404 scenarios.
19-49: LGTM!The factory implementation correctly generates deterministic agreement payloads with time-frozen dates and comprehensive field coverage.
| def factory( | ||
| name: str = "E2E Created Agreement", | ||
| client_external_id: str = "test-client-external-id", | ||
| vendor_external_id: str = "test-vendor-external-id", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove type annotations from test code.
Per past review comments, this repository doesn't use type annotations in test files. Remove the type hints from the factory function parameters.
Apply this diff:
- def factory(
- name: str = "E2E Created Agreement",
- client_external_id: str = "test-client-external-id",
- vendor_external_id: str = "test-vendor-external-id",
- ):
+ def factory(
+ name="E2E Created Agreement",
+ client_external_id="test-client-external-id",
+ vendor_external_id="test-vendor-external-id",
+ ):Based on learnings, past review comments indicated that type annotations should not be used in test files in this repository.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def factory( | |
| name: str = "E2E Created Agreement", | |
| client_external_id: str = "test-client-external-id", | |
| vendor_external_id: str = "test-vendor-external-id", | |
| def factory( | |
| name="E2E Created Agreement", | |
| client_external_id="test-client-external-id", | |
| vendor_external_id="test-vendor-external-id", |
🤖 Prompt for AI Agents
In tests/e2e/commerce/agreement/conftest.py around lines 20 to 23, the factory
function parameters currently include Python type annotations (e.g., name: str,
client_external_id: str, vendor_external_id: str); remove all type annotations
from the test function signature so parameters are untyped (e.g., name = "E2E
Created Agreement", client_external_id = "test-client-external-id",
vendor_external_id = "test-vendor-external-id") while leaving default values
intact and adjusting any related docstrings or calls if necessary.
|
robcsegal
deleted the
MPT-14904-add-e-2-e-tests-for-commerce-agreements
branch
Created e2e tests for commerce agreements
Moved some id fixtures from accounts because they're shared with commerce to avoid unnecessary duplication.
https://softwareone.atlassian.net/browse/MPT-14904
Summary by CodeRabbit
New Features
Tests
Configuration & Fixtures
Chores
✏️ Tip: You can customize this high-level summary in your review settings.