[MPT-14915] Added e2e tests for billing ledger, charges, and ledger attachments #186
Conversation
📝 WalkthroughWalkthroughAdds three e2e config keys and a relocated commerce fixture; introduces pytest fixtures and new sync/async end-to-end tests for billing ledgers, charges, and attachments (CRUD, filtering, listing, download), plus related conftest fixtures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In @tests/e2e/billing/ledger/attachment/test_async_ledger_attachment.py:
- Around line 53-63: The test_filter_ledger_attachments currently filters by a
hardcoded name which may not match the existing resource identified by
ledger_attachment_id; update the test to use the created fixture or remove the
name filter: either replace the name filter with
RQLQuery(name=created_ledger_attachment.name) by adding the
created_ledger_attachment fixture to the test signature, or delete the
.filter(RQLQuery(name="E2E Seeded Ledger Attachment")) call so the test only
filters by RQLQuery(id=ledger_attachment_id); keep references to
ledger_attachments, ledger_attachment_id, and test_filter_ledger_attachments
when making the change.
In @tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:
- Around line 33-43: The test test_filter_ledger_charges is using a hardcoded
externalIds__invoice="INV12345" filter which may not match the fixture
referenced by ledger_charge_id; remove the
.filter(RQLQuery(externalIds__invoice="INV12345")) call so the test only filters
by id via ledger_charges.filter(RQLQuery(id=ledger_charge_id)), or replace the
hardcoded value with the actual external invoice value from the test fixture
(e.g., fetch invoice = ledger_charge_fixture.externalIds.get("invoice") and use
.filter(RQLQuery(externalIds__invoice=invoice))). Ensure references to
ledger_charges, ledger_charge_id and externalIds__invoice are updated
accordingly.
🧹 Nitpick comments (1)
tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py (1)
83-87: Avoid double deletion in test and fixture teardown.The test explicitly deletes the attachment (line 86), and then the
created_ledger_attachmentfixture teardown will attempt to delete it again (line 23). While the teardown catches the error, this creates unnecessary error logging.Consider one of these approaches:
- Skip the teardown if the test deletes (use a flag or yield a tuple)
- Remove the deletion from the test body and rely solely on fixture cleanup
♻️ Option 1: Modify test to skip fixture teardown
def test_delete_billing_ledger_attachment(ledger_attachments, created_ledger_attachment): result = created_ledger_attachment ledger_attachments.delete(result.id) + # Mark as already deleted to skip fixture teardown + created_ledger_attachment._deleted = TrueAnd update the fixture:
yield created_ledger_attachment - try: - ledger_attachments.delete(created_ledger_attachment.id) - except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete ledger attachment: {error.title}") + if not getattr(created_ledger_attachment, '_deleted', False): + try: + ledger_attachments.delete(created_ledger_attachment.id) + except MPTAPIError as error: + print(f"TEARDOWN - Unable to delete ledger attachment: {error.title}")
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
e2e_config.test.jsonpyproject.tomltests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/attachment/test_async_ledger_attachment.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/conftest.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/commerce/conftest.pytests/e2e/conftest.py
💤 Files with no reviewable changes (1)
- tests/e2e/commerce/conftest.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not suggest formatting changes or mention code formatting using Black. Instead use Ruff or Flake8
Files:
tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/attachment/test_async_ledger_attachment.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/conftest.pytests/e2e/billing/ledger/conftest.py
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing subsequent commits, always check if previous review comments have been addressed and acknowledge resolved issues.
Files:
tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pye2e_config.test.jsontests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/attachment/test_async_ledger_attachment.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/conftest.pytests/e2e/billing/ledger/conftest.pypyproject.toml
🧠 Learnings (4)
📚 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/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/attachment/test_async_ledger_attachment.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/conftest.pytests/e2e/billing/ledger/conftest.py
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.
Applied to files:
tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/attachment/test_async_ledger_attachment.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/conftest.pytests/e2e/billing/ledger/conftest.py
📚 Learning: 2026-01-08T08:34:19.306Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/items/conftest.py:26-27
Timestamp: 2026-01-08T08:34:19.306Z
Learning: In the e2e tests, price list items are manually seeded for testing purposes, so the price_list_item fixture in tests/e2e/catalog/price_lists/items/conftest.py is expected to always find an item and doesn't require additional error handling for StopIteration.
Applied to files:
tests/e2e/conftest.py
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
pyproject.toml
🧬 Code graph analysis (5)
tests/e2e/billing/ledger/test_async_ledger.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/billing/ledger/test_sync_ledger.py (4)
test_get_billing_ledger_by_id(9-12)test_list_billing_ledgers(15-20)test_get_billing_ledger_by_id_not_found(23-25)test_filter_billing_ledgers(28-38)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)commerce_product_id(162-163)tests/e2e/billing/ledger/conftest.py (2)
ledger_id(5-6)invalid_ledger_id(10-11)
tests/e2e/billing/ledger/attachment/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/billing/ledger/charge/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/billing/ledger/attachment/test_async_ledger_attachment.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 (2)
async_mpt_ops(36-39)pdf_fd(73-75)tests/e2e/billing/ledger/attachment/conftest.py (1)
ledger_attachment_factory(15-24)tests/e2e/billing/ledger/conftest.py (1)
ledger_id(5-6)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
tests/e2e/billing/ledger/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
⏰ 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 (25)
pyproject.toml (1)
129-129: LGTM!The per-file-ignores entry for billing e2e tests follows the established pattern for other e2e test directories and appropriately silences WPS202 (module complexity) and WPS421 (function call warnings) which are common in test files with many fixtures.
e2e_config.test.json (1)
17-19: LGTM!The new billing ledger configuration keys follow the existing naming conventions and ID format patterns. They're correctly positioned within the billing namespace and align with the new e2e test fixtures introduced in this PR.
tests/e2e/conftest.py (1)
161-163: LGTM!The
commerce_product_idfixture is correctly implemented and follows the established pattern of other ID fixtures in this file. Centralizing it at the root conftest level appropriately makes it available to the new billing ledger tests while maintaining consistency with the existing codebase structure.tests/e2e/billing/ledger/charge/conftest.py (2)
4-6: LGTM!The
ledger_charge_idfixture correctly reads from the e2e configuration and follows the established pattern of other ID fixtures in the test suite.
9-11: LGTM!The
invalid_ledger_charge_idfixture provides a hardcoded invalid ID for negative testing, which is an appropriate pattern for testing error cases and not-found scenarios.tests/e2e/billing/ledger/conftest.py (2)
4-6: LGTM!The
ledger_idfixture correctly reads from the e2e configuration and follows the established pattern consistently used across other e2e test fixtures.
9-11: LGTM!The
invalid_ledger_idfixture provides a hardcoded invalid ID for negative testing scenarios, which is the appropriate pattern for testing not-found cases and error handling.tests/e2e/billing/ledger/attachment/test_async_ledger_attachment.py (8)
9-24: LGTM!The fixture properly creates and tears down the ledger attachment resource, with appropriate error handling during cleanup. This follows the established pattern for e2e test fixtures that require per-test creation and cleanup.
27-29: LGTM!The fixture appropriately provides a reusable client accessor for ledger attachments, improving test readability.
32-42: LGTM!The tests correctly validate both successful retrieval and error handling for invalid IDs using the appropriate async patterns.
45-51: LGTM!The pagination test correctly validates that attachments can be listed.
66-69: LGTM!The test correctly uses a synchronous function signature for a test that relies on async fixtures without performing any awaits in the test body itself, following the established pattern.
72-81: LGTM!The update test correctly modifies the isolated attachment resource and validates the operation.
84-87: LGTM!The delete test validates that the deletion operation completes without errors. The fixture's teardown handles cleanup gracefully if the resource is already deleted.
90-93: LGTM!The download test correctly validates both file contents and filename extraction from the response.
tests/e2e/billing/ledger/attachment/conftest.py (1)
1-24: LGTM!The fixtures follow established e2e test patterns, providing both config-driven and factory-based test data for ledger attachment operations.
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py (3)
9-11: LGTM!The fixture appropriately provides a reusable sync client accessor for ledger charges.
14-22: LGTM!The tests correctly validate charge retrieval and error handling using synchronous API calls.
25-30: LGTM!The pagination test correctly validates that charges can be listed.
tests/e2e/billing/ledger/test_sync_ledger.py (1)
9-38: LGTM!The synchronous ledger tests correctly validate retrieval, listing, error handling, and filtering operations. The use of config-driven fixtures ensures tests run against known resources.
tests/e2e/billing/ledger/test_async_ledger.py (1)
9-38: LGTM!The asynchronous ledger tests correctly implement async patterns with proper use of
awaitfor API calls and async iteration. The tests provide comprehensive coverage of ledger operations.tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py (2)
1-93: Overall test structure looks solid.The test module follows established E2E patterns with:
- Proper use of isolated fixtures for destructive operations
- Reuse of seeded resources for read-only operations
- Comprehensive coverage of CRUD operations plus download functionality
52-63: The hardcoded name "E2E Seeded Ledger Attachment" cannot be verified against the pre-seeded attachment.The test filters the pre-seeded attachment (ID from config:
ledger_attachment_id) by a hardcoded name"E2E Seeded Ledger Attachment". However, this name is only confirmed to be used when creating new attachments via thecreated_ledger_attachmentfixture. The e2e config specifies only the attachment ID ("LEA-4971-4321"), not its name. Verify that the pre-seeded attachment actually has this exact name, or update the test to use thecreated_ledger_attachmentfixture instead of filtering by a hardcoded value.tests/e2e/billing/ledger/charge/test_async_ledger_charge.py (2)
14-43: Async test implementation looks correct.All async test functions properly use
awaitfor API calls and async comprehension for iteration. The read-only nature of these tests (no create/update/delete) allows efficient reuse of seeded resources.
33-43: The test is valid as written. The hardcoded invoice external ID"INV12345"is explicitly seeded in the test data (tests/data/test_billing_journal.jsonl) with"externalIds": {"invoice": "INV12345", ...}, so the filter will correctly return results. The multiple filter chaining (byidandexternalIds__invoice) is intentional for testing nested field filtering functionality.
robcsegal
force-pushed
the
MPT-14915-add-e-2-e-tests-for-billing-ledgers
branch
from
dac7180 to
0510716
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
116-132: Remove the duplicate per-file-ignore; WPS118 is still ignored.Because Line 129 is still present, Flake8 will continue ignoring
WPS118fortests/e2e/billing/*.py, defeating the intent of Line 130.Proposed fix
per-file-ignores = [ @@ - "tests/e2e/billing/*.py: WPS202 WPS421 WPS118", "tests/e2e/billing/*.py: WPS202 WPS421", "tests/e2e/catalog/*.py: WPS202 WPS421",
🤖 Fix all issues with AI agents
In @tests/e2e/billing/ledger/charge/conftest.py:
- Around line 4-11: The invalid_ledger_charge_id fixture returns an ID that
doesn't match the expected "CHG-..." shape and can trigger validation (400)
instead of not-found (404); update the invalid_ledger_charge_id fixture to
return a same-shaped but nonexistent ID (e.g., "CHG-0000-0000") so tests
exercise the 404 path, and ensure this aligns with the format used by
e2e_config/test data (see ledger_charge_id and e2e_config).
In @tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:
- Around line 33-39: The test_filter_ledger_charges test currently hardcodes
externalIds__invoice="INV12345", which may not exist in seeded data; update the
test to derive the invoice value instead of hardcoding it: either read the
invoice from the shared test config (e2e_config) and use that variable in the
RQLQuery, or first fetch the charge via ledger_charges.get(id=ledger_charge_id)
(or ledger_charges.filter(RQLQuery(id=ledger_charge_id)).first()) to extract the
actual externalIds.invoice value and then call
ledger_charges.filter(RQLQuery(externalIds__invoice=<derived_invoice>)).select(...).
Ensure you reference the existing test function test_filter_ledger_charges, the
ledger_charges fixture, and RQLQuery when making the change.
🧹 Nitpick comments (4)
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py (2)
20-22: Make the 404 assertion less brittle (or assert on status code).Depending on how
MPTAPIError.__str__()formats payloads, matching the full"404 Not Found"may be flaky; consider matching just\b404\bor asserting on an exposed status/status_code attribute.
25-31: List test can legitimately return empty; assert discoverability of the seeded charge instead.If the ledger has no charges (or pagination/permissions differ),
len(result) > 0will fail. Since you already seedledger_charge_id, consider asserting that listing/iteration can find that specific ID (possibly with a higher limit).tests/e2e/billing/ledger/attachment/conftest.py (1)
14-24: Consider making the default attachment name unique (or ensure callers always pass a unique name + cleanup).Given these are e2e tests and attachments may be created, a static default like
"E2E Created Ledger Attachment"can collide across retries/reruns. Based on learnings, if this fixture is used for destructive create/delete flows, ensure tests clearly clean up created attachments (or generate unique names by default).tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py (1)
83-86: Consider verifying deletion succeeded.The test deletes the attachment but doesn't verify that it was actually removed. Consider adding a follow-up assertion to confirm the resource is no longer accessible.
♻️ Optional enhancement to verify deletion
def test_delete_billing_ledger_attachment(ledger_attachments, created_ledger_attachment): result = created_ledger_attachment ledger_attachments.delete(result.id) + + # Verify deletion succeeded + with pytest.raises(MPTAPIError, match=r"404 Not Found"): + ledger_attachments.get(result.id)
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
e2e_config.test.jsonpyproject.tomltests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/attachment/test_async_ledger_attachment.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/conftest.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/commerce/conftest.pytests/e2e/conftest.py
💤 Files with no reviewable changes (1)
- tests/e2e/commerce/conftest.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/conftest.py
- tests/e2e/billing/ledger/attachment/test_async_ledger_attachment.py
- tests/e2e/billing/ledger/conftest.py
- tests/e2e/billing/ledger/test_async_ledger.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not suggest formatting changes or mention code formatting using Black. Instead use Ruff or Flake8
Files:
tests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/attachment/conftest.py
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing subsequent commits, always check if previous review comments have been addressed and acknowledge resolved issues.
Files:
tests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/attachment/conftest.pypyproject.tomle2e_config.test.json
🧠 Learnings (5)
📓 Common learnings
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In the mpt-api-python-client E2E tests, the team intentionally reuses existing API resources (via e2e_config) for read-only operations and safe mutations (e.g., updating the "notes" field) to improve test execution speed. Only destructive/dangerous tests use isolated fixtures (created_price_list or async_created_price_list) that are created and cleaned up per test.
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.
Applied to files:
tests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/attachment/conftest.py
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/attachment/conftest.py
📚 Learning: 2026-01-08T08:34:19.306Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/items/conftest.py:26-27
Timestamp: 2026-01-08T08:34:19.306Z
Learning: In the e2e tests, price list items are manually seeded for testing purposes, so the price_list_item fixture in tests/e2e/catalog/price_lists/items/conftest.py is expected to always find an item and doesn't require additional error handling for StopIteration.
Applied to files:
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
pyproject.toml
🧬 Code graph analysis (2)
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.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)
mpt_ops(31-32)tests/e2e/billing/ledger/conftest.py (1)
ledger_id(5-6)tests/e2e/billing/ledger/charge/conftest.py (2)
ledger_charge_id(5-6)invalid_ledger_charge_id(10-11)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py (8)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/billing/ledger/attachment/test_async_ledger_attachment.py (1)
created_ledger_attachment(10-24)tests/e2e/conftest.py (1)
pdf_fd(73-75)tests/e2e/billing/ledger/attachment/conftest.py (3)
ledger_attachment_factory(15-24)ledger_attachment_id(10-11)invalid_ledger_attachment_id(5-6)tests/e2e/billing/ledger/conftest.py (1)
ledger_id(5-6)mpt_api_client/models/model.py (1)
id(119-121)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
⏰ 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 (6)
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py (1)
9-11: Good: ledger-scoped fixture matches the resource hierarchy.
mpt_ops.billing.ledgers.charges(ledger_id)keeps tests aligned with the API surface (ledger → charges).e2e_config.test.json (1)
20-22: ID coherence between ledger and charge/attachment requires correct seeded data.The three new keys are actively used in test fixtures (
ledger_idscopes the charges API call). However, ensure the test environment has seeded the charge and attachment with IDs that actually belong to the ledger specified inbilling.ledger.id—the API will reject requests if they don't match (e.g.,ledger_charges.get(ledger_charge_id)will 404 if the charge isn't owned by the ledger).tests/e2e/billing/ledger/test_sync_ledger.py (1)
1-38: LGTM! Well-structured E2E tests for billing ledgers.The test module provides good coverage of core ledger operations including retrieval, pagination, error handling, and RQL-based filtering. The tests appropriately reuse existing resources via fixtures (following team practices for E2E test performance), and the flaky marker is correctly applied for E2E test stability.
tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py (2)
9-25: LGTM! Proper fixture lifecycle management.The
created_ledger_attachmentfixture correctly implements creation and teardown with appropriate error handling during cleanup. This follows established patterns for E2E test fixtures.
89-92: LGTM! Appropriate download validation.The test correctly validates both
file_contentsandfilenameare present in the download response, which are the key properties exposed by the FileModel.tests/e2e/billing/ledger/charge/test_async_ledger_charge.py (1)
1-43: LGTM! Properly structured async E2E tests.The async test module correctly follows pytest-asyncio patterns with:
- Synchronous fixture returning the async client (lines 9-11)
- Async test functions properly using
awaitfor API calls- Async iteration for filtered results (line 41)
The tests provide good coverage of ledger charge operations including retrieval, pagination, error handling, and RQL-based filtering.
robcsegal
force-pushed
the
MPT-14915-add-e-2-e-tests-for-billing-ledgers
branch
from
0510716 to
b74f853
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pyproject.toml:
- Around line 129-130: The per-file-ignores section contains a duplicate entry
for the pattern tests/e2e/billing/*.py where the second line (with "WPS202
WPS421") overrides the first and drops WPS118; remove the duplicate or merge
them into a single per-file-ignores entry for tests/e2e/billing/*.py that
includes WPS118 (e.g., keep "WPS202 WPS421 WPS118") so long test names in
billing tests no longer trigger WPS118.
🧹 Nitpick comments (1)
tests/e2e/billing/ledger/test_async_ledger.py (1)
28-38: Consider a more precise assertion.The test correctly demonstrates filtering, field selection, and async iteration. Since the filter targets a specific
ledger_id, the assertion could be more precise by expecting exactly one result rather than just checking that results exist.💡 Optional enhancement for assertion precision
result = [ledger async for ledger in filtered_ledgers.iterate()] - assert len(result) > 0 + assert len(result) == 1 + assert result[0].id == ledger_id
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
e2e_config.test.jsonpyproject.tomltests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/attachment/test_async_ledger_attachment.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/conftest.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/commerce/conftest.pytests/e2e/conftest.py
💤 Files with no reviewable changes (1)
- tests/e2e/commerce/conftest.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/conftest.py
- tests/e2e/billing/ledger/charge/conftest.py
- tests/e2e/billing/ledger/conftest.py
- tests/e2e/billing/ledger/attachment/test_async_ledger_attachment.py
- tests/e2e/billing/ledger/charge/test_async_ledger_charge.py
- tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not suggest formatting changes or mention code formatting using Black. Instead use Ruff or Flake8
Files:
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/attachment/conftest.py
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing subsequent commits, always check if previous review comments have been addressed and acknowledge resolved issues.
Files:
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/test_async_ledger.pye2e_config.test.jsontests/e2e/billing/ledger/test_sync_ledger.pypyproject.tomltests/e2e/billing/ledger/attachment/conftest.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In the mpt-api-python-client E2E tests, the team intentionally reuses existing API resources (via e2e_config) for read-only operations and safe mutations (e.g., updating the "notes" field) to improve test execution speed. Only destructive/dangerous tests use isolated fixtures (created_price_list or async_created_price_list) that are created and cleaned up per test.
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.
Applied to files:
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/attachment/conftest.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.
Applied to files:
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/attachment/conftest.py
📚 Learning: 2026-01-08T08:34:19.306Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/items/conftest.py:26-27
Timestamp: 2026-01-08T08:34:19.306Z
Learning: In the e2e tests, price list items are manually seeded for testing purposes, so the price_list_item fixture in tests/e2e/catalog/price_lists/items/conftest.py is expected to always find an item and doesn't require additional error handling for StopIteration.
Applied to files:
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.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/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/billing/ledger/attachment/conftest.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In the mpt-api-python-client e2e tests, hardcoded values like "INV12345" for invoice external IDs are intentional and guaranteed to exist because they come from import files used to seed the test data.
Applied to files:
e2e_config.test.json
📚 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 (3)
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/billing/ledger/conftest.py (1)
ledger_id(5-6)tests/e2e/billing/ledger/charge/conftest.py (2)
ledger_charge_id(5-6)invalid_ledger_charge_id(10-11)
tests/e2e/billing/ledger/test_sync_ledger.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)
mpt_ops(31-32)commerce_product_id(162-163)tests/e2e/billing/ledger/conftest.py (2)
ledger_id(5-6)invalid_ledger_id(10-11)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/billing/ledger/attachment/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
⏰ 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 (15)
e2e_config.test.json (1)
20-22: LGTM! Configuration keys follow established patterns.The new billing ledger configuration keys are consistent with existing naming conventions and ID formats in the config file.
tests/e2e/billing/ledger/attachment/conftest.py (1)
1-24: LGTM! Fixtures follow established e2e test patterns.The fixtures are consistent with existing e2e test patterns and align with the team's approach of reusing existing API resources from e2e_config.
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py (5)
1-11: LGTM! Module setup follows e2e test conventions.The flaky marker and fixture setup are appropriate for end-to-end tests.
14-17: LGTM! Standard retrieval test.The test correctly validates ledger charge retrieval by ID.
20-22: LGTM! Appropriate 404 handling test.The test correctly validates error handling for non-existent ledger charges.
25-30: LGTM! Standard pagination test.The test correctly validates ledger charge listing with pagination.
33-43: LGTM! Filter test correctly uses seeded test data.The hardcoded invoice external ID "INV12345" is intentional and comes from seeded test data. Based on learnings, this approach is acceptable for e2e tests that rely on pre-populated data.
tests/e2e/billing/ledger/test_sync_ledger.py (4)
1-6: LGTM! Module setup is appropriate.The imports and flaky marker follow e2e test conventions.
9-12: LGTM! Standard retrieval test.The test correctly validates billing ledger retrieval by ID.
15-25: LGTM! Standard pagination and error handling tests.Both tests correctly validate ledger listing and 404 error scenarios.
28-38: LGTM! Filter test correctly combines multiple filters.The test appropriately filters ledgers by ID and product, using the commerce_product_id fixture. The projection excludes the authorization field as expected.
tests/e2e/billing/ledger/test_async_ledger.py (4)
1-6: LGTM! Clean imports and appropriate test markers.The imports are well-chosen, and marking all tests as flaky is a sensible approach for end-to-end tests.
9-12: LGTM! Straightforward read operation test.This test appropriately verifies that a ledger can be retrieved by ID. The minimal assertion is suitable for e2e testing.
15-20: LGTM! Basic pagination test.This test correctly verifies that ledgers can be fetched with pagination.
23-25: LGTM! Well-implemented negative test case.The test correctly verifies error handling for non-existent ledgers.
robcsegal
force-pushed
the
MPT-14915-add-e-2-e-tests-for-billing-ledgers
branch
from
b74f853 to
d51286a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/e2e/billing/ledger/conftest.py:
- Around line 9-11: The invalid_ledger_id fixture returns an ID with the wrong
prefix; update the fixture invalid_ledger_id to return an ID string using the
valid prefix "BLE-" (e.g., "BLE-0000-0000") so tests expecting BLE- prefixed
ledger IDs exercise the intended error paths and match the configured pattern.
🧹 Nitpick comments (8)
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py (2)
14-17: Consider more specific assertions.While the test is functional, consider asserting that
result.id == ledger_charge_idto verify the correct charge was retrieved.💡 Optional improvement
def test_get_ledger_charge_by_id(ledger_charges, ledger_charge_id): result = ledger_charges.get(ledger_charge_id) - assert result is not None + assert result is not None + assert result.id == ledger_charge_id
33-42: Extract hardcoded external ID to a named constant.The hardcoded
"INV12345"depends on seeded test data. Per coding patterns for this codebase, prefer extracting such IDs to named constants at module level and document the dependency on seed data consistency.Based on learnings, this improves maintainability when seed data changes.
♻️ Proposed refactor
At the top of the file, after imports:
pytestmark = [pytest.mark.flaky] +# External IDs from seeded test data +SEEDED_INVOICE_EXTERNAL_ID = "INV12345" +Then update the test:
def test_filter_ledger_charges(ledger_charges, ledger_charge_id): select_fields = ["-journal"] filtered_charges = ( ledger_charges.filter(RQLQuery(id=ledger_charge_id)) - .filter(RQLQuery(externalIds__invoice="INV12345")) + .filter(RQLQuery(externalIds__invoice=SEEDED_INVOICE_EXTERNAL_ID)) .select(*select_fields) )tests/e2e/billing/ledger/charge/test_async_ledger_charge.py (2)
14-17: Consider more specific assertions.While the test is functional, consider asserting that
result.id == ledger_charge_idto verify the correct charge was retrieved.💡 Optional improvement
async def test_get_ledger_charge_by_id(ledger_charges, ledger_charge_id): result = await ledger_charges.get(ledger_charge_id) - assert result is not None + assert result is not None + assert result.id == ledger_charge_id
33-42: Extract hardcoded external ID to a named constant.The hardcoded
"INV12345"depends on seeded test data. Per coding patterns for this codebase, prefer extracting such IDs to named constants at module level and document the dependency on seed data consistency.Based on learnings, this improves maintainability when seed data changes.
♻️ Proposed refactor
At the top of the file, after imports:
pytestmark = [pytest.mark.flaky] +# External IDs from seeded test data +SEEDED_INVOICE_EXTERNAL_ID = "INV12345" +Then update the test:
async def test_filter_ledger_charges(ledger_charges, ledger_charge_id): select_fields = ["-journal"] filtered_charges = ( ledger_charges.filter(RQLQuery(id=ledger_charge_id)) - .filter(RQLQuery(externalIds__invoice="INV12345")) + .filter(RQLQuery(externalIds__invoice=SEEDED_INVOICE_EXTERNAL_ID)) .select(*select_fields) )tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py (4)
33-36: Consider more specific assertions.While the test is functional, consider asserting that
result.id == ledger_attachment_idto verify the correct attachment was retrieved.💡 Optional improvement
def test_get_ledger_attachment_by_id(ledger_attachments, ledger_attachment_id): result = ledger_attachments.get(ledger_attachment_id) - assert result is not None + assert result is not None + assert result.id == ledger_attachment_id
52-62: Extract hardcoded seeded data name to a named constant.The hardcoded
"E2E Seeded Ledger Attachment"depends on seeded test data. Per coding patterns for this codebase, prefer extracting such values to named constants at module level and document the dependency on seed data consistency.Based on learnings, this improves maintainability when seed data changes.
♻️ Proposed refactor
At the top of the file, after imports:
pytestmark = [pytest.mark.flaky] +# Names from seeded test data +SEEDED_ATTACHMENT_NAME = "E2E Seeded Ledger Attachment" +Then update the test:
def test_filter_ledger_attachments(ledger_attachments, ledger_attachment_id): select_fields = ["-description"] filtered_attachments = ( ledger_attachments.filter(RQLQuery(id=ledger_attachment_id)) - .filter(RQLQuery(name="E2E Seeded Ledger Attachment")) + .filter(RQLQuery(name=SEEDED_ATTACHMENT_NAME)) .select(*select_fields) )
65-68: Consider asserting creation details.While the test validates creation succeeded, consider asserting specific fields like
result.name == "E2E Created Ledger Attachment"to verify the attachment was created with the expected data.💡 Optional improvement
def test_create_billing_ledger_attachment(created_ledger_attachment): result = created_ledger_attachment assert result is not None + assert result.name == "E2E Created Ledger Attachment"
71-80: Consider asserting update details.While the test validates the update succeeded, consider asserting that
result.name == "E2E Updated Ledger Attachment"to verify the attachment was updated with the expected data.💡 Optional improvement
def test_update_billing_ledger_attachment( ledger_attachments, created_ledger_attachment, ledger_attachment_factory ): updated_data = ledger_attachment_factory( name="E2E Updated Ledger Attachment", ) result = ledger_attachments.update(created_ledger_attachment.id, updated_data) assert result is not None + assert result.name == "E2E Updated Ledger Attachment"
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e_config.test.jsontests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/attachment/test_async_ledger_attachment.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/conftest.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/commerce/conftest.pytests/e2e/conftest.py
💤 Files with no reviewable changes (1)
- tests/e2e/commerce/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/billing/ledger/charge/conftest.py
- tests/e2e/billing/ledger/test_sync_ledger.py
- tests/e2e/billing/ledger/attachment/test_async_ledger_attachment.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not suggest formatting changes or mention code formatting using Black. Instead use Ruff or Flake8
Files:
tests/e2e/conftest.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/conftest.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing subsequent commits, always check if previous review comments have been addressed and acknowledge resolved issues.
Files:
tests/e2e/conftest.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/conftest.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pye2e_config.test.json
🧠 Learnings (6)
📓 Common learnings
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In the mpt-api-python-client E2E tests, the team intentionally reuses existing API resources (via e2e_config) for read-only operations and safe mutations (e.g., updating the "notes" field) to improve test execution speed. Only destructive/dangerous tests use isolated fixtures (created_price_list or async_created_price_list) that are created and cleaned up per test.
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.
Applied to files:
tests/e2e/conftest.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/conftest.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py
📚 Learning: 2026-01-08T08:34:19.306Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/items/conftest.py:26-27
Timestamp: 2026-01-08T08:34:19.306Z
Learning: In the e2e tests, price list items are manually seeded for testing purposes, so the price_list_item fixture in tests/e2e/catalog/price_lists/items/conftest.py is expected to always find an item and doesn't require additional error handling for StopIteration.
Applied to files:
tests/e2e/conftest.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.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/conftest.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/conftest.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.
Applied to files:
tests/e2e/conftest.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/conftest.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In the mpt-api-python-client e2e tests, hardcoded values like "INV12345" for invoice external IDs are intentional and guaranteed to exist because they come from import files used to seed the test data.
Applied to files:
e2e_config.test.json
🧬 Code graph analysis (5)
tests/e2e/billing/ledger/attachment/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/billing/ledger/test_async_ledger.py (2)
tests/e2e/conftest.py (2)
async_mpt_ops(36-39)commerce_product_id(162-163)tests/e2e/billing/ledger/conftest.py (2)
ledger_id(5-6)invalid_ledger_id(10-11)
tests/e2e/billing/ledger/charge/test_async_ledger_charge.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 (1)
async_mpt_ops(36-39)tests/e2e/billing/ledger/conftest.py (1)
ledger_id(5-6)tests/e2e/billing/ledger/charge/conftest.py (2)
ledger_charge_id(5-6)invalid_ledger_charge_id(10-11)
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.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/billing/ledger/charge/conftest.py (2)
ledger_charge_id(5-6)invalid_ledger_charge_id(10-11)
tests/e2e/billing/ledger/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
⏰ 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 (26)
tests/e2e/conftest.py (1)
161-163: LGTM! Fixture centralization improves reusability.The
commerce_product_idfixture follows the established pattern and centralizes access to the commerce product ID for use across multiple test modules.e2e_config.test.json (1)
20-22: LGTM! Configuration keys properly added.The new billing ledger configuration keys follow the established naming pattern and provide the necessary IDs for the new test suites.
tests/e2e/billing/ledger/conftest.py (1)
4-6: LGTM! Fixture correctly sources ledger ID from configuration.The
ledger_idfixture follows the established pattern for accessing test configuration values.tests/e2e/billing/ledger/attachment/conftest.py (3)
4-6: LGTM! Invalid ID fixture follows correct pattern.The invalid ledger attachment ID correctly uses the
LEA-prefix matching the valid attachment ID pattern.
9-11: LGTM! Fixture correctly sources attachment ID from configuration.The
ledger_attachment_idfixture follows the established pattern for accessing test configuration values.
14-24: LGTM! Well-designed factory fixture.The
ledger_attachment_factoryprovides a clean, reusable pattern for generating ledger attachment data with sensible defaults.tests/e2e/billing/ledger/test_async_ledger.py (5)
1-6: LGTM! Appropriate setup for E2E tests.The imports are correct and marking all tests as flaky is a reasonable approach for end-to-end tests that may experience intermittent failures due to external dependencies.
9-13: LGTM! Basic retrieval test is appropriate.The test appropriately validates ledger retrieval functionality. The lightweight assertion is suitable for E2E tests.
15-21: LGTM! Pagination test is well-structured.The test appropriately validates the listing functionality with pagination.
23-26: LGTM! Error handling test is appropriate.The test correctly validates 404 error handling for non-existent ledgers. Note: ensure the
invalid_ledger_idfixture uses the correct prefix (see comment on tests/e2e/billing/ledger/conftest.py).
28-38: LGTM! Filtering test demonstrates proper RQL usage.The test appropriately validates filtering with RQL queries and field selection. The async iteration pattern is correctly implemented.
tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py (4)
1-6: LGTM!The imports and flaky marker are appropriate for these end-to-end tests.
9-11: LGTM!The fixture correctly returns a read-only charges object, aligning with the pattern of reusing existing resources for read-only operations.
20-22: LGTM!The 404 error handling test correctly validates the expected exception behavior.
25-30: LGTM!The list test correctly validates that charges can be fetched and the result is non-empty.
tests/e2e/billing/ledger/charge/test_async_ledger_charge.py (4)
1-6: LGTM!The imports and flaky marker are appropriate for these asynchronous end-to-end tests.
9-11: LGTM!The fixture correctly returns an async charges object. The fixture itself doesn't need to be async since it's only returning a reference.
20-22: LGTM!The async 404 error handling test correctly validates the expected exception behavior.
25-30: LGTM!The async list test correctly validates that charges can be fetched asynchronously and the result is non-empty.
tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py (7)
1-6: LGTM!The imports and flaky marker are appropriate for these end-to-end tests.
9-25: LGTM!The fixture correctly creates an isolated attachment for destructive tests and handles cleanup appropriately. The error handling in teardown prevents test failures if the attachment was already deleted by the test itself (e.g., in
test_delete_billing_ledger_attachment).
28-30: LGTM!The fixture correctly returns a read-only attachments object, aligning with the pattern of reusing existing resources for read-only operations.
39-41: LGTM!The 404 error handling test correctly validates the expected exception behavior.
44-49: LGTM!The list test correctly validates that attachments can be fetched and the result is non-empty.
83-86: LGTM!The test correctly validates the delete operation. The fixture's teardown will gracefully handle the attachment already being deleted.
89-92: LGTM!The download test correctly validates that both file contents and filename are returned.
robcsegal
force-pushed
the
MPT-14915-add-e-2-e-tests-for-billing-ledgers
branch
from
d51286a to
c16658a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/billing/ledger/test_sync_ledger.py (1)
28-38: Consider using consistent RQLQuery pattern for clarity.The test mixes two RQLQuery construction styles: kwargs on line 31 (
RQLQuery(id=ledger_id)) and a raw string on line 32 (RQLQuery(f"product.id={commerce_product_id}")). For consistency, consider using dot notation:♻️ Suggested refactor
def test_filter_billing_ledgers(mpt_ops, ledger_id, commerce_product_id): select_fields = ["-authorization"] filtered_ledgers = ( mpt_ops.billing.ledgers.filter(RQLQuery(id=ledger_id)) - .filter(RQLQuery(f"product.id={commerce_product_id}")) + .filter(RQLQuery().product.id.eq(commerce_product_id)) .select(*select_fields) )
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e_config.test.jsontests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/attachment/test_async_ledger_attachment.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/charge/conftest.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/charge/test_sync_ledger_charge.pytests/e2e/billing/ledger/conftest.pytests/e2e/billing/ledger/test_async_ledger.pytests/e2e/billing/ledger/test_sync_ledger.pytests/e2e/commerce/conftest.pytests/e2e/conftest.py
💤 Files with no reviewable changes (1)
- tests/e2e/commerce/conftest.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/billing/ledger/test_async_ledger.py
- tests/e2e/billing/ledger/charge/conftest.py
- tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py
- e2e_config.test.json
- tests/e2e/billing/ledger/attachment/test_async_ledger_attachment.py
- tests/e2e/billing/ledger/conftest.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not suggest formatting changes or mention code formatting using Black. Instead use Ruff or Flake8
Files:
tests/e2e/conftest.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/test_sync_ledger.py
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing subsequent commits, always check if previous review comments have been addressed and acknowledge resolved issues.
Files:
tests/e2e/conftest.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/test_sync_ledger.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In the mpt-api-python-client E2E tests, the team intentionally reuses existing API resources (via e2e_config) for read-only operations and safe mutations (e.g., updating the "notes" field) to improve test execution speed. Only destructive/dangerous tests use isolated fixtures (created_price_list or async_created_price_list) that are created and cleaned up per test.
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.
Applied to files:
tests/e2e/conftest.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/test_sync_ledger.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/conftest.py
📚 Learning: 2026-01-08T08:34:19.306Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/items/conftest.py:26-27
Timestamp: 2026-01-08T08:34:19.306Z
Learning: In the e2e tests, price list items are manually seeded for testing purposes, so the price_list_item fixture in tests/e2e/catalog/price_lists/items/conftest.py is expected to always find an item and doesn't require additional error handling for StopIteration.
Applied to files:
tests/e2e/conftest.py
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/conftest.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/test_sync_ledger.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.
Applied to files:
tests/e2e/conftest.pytests/e2e/billing/ledger/charge/test_async_ledger_charge.pytests/e2e/billing/ledger/attachment/conftest.pytests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.pytests/e2e/billing/ledger/test_sync_ledger.py
🧬 Code graph analysis (3)
tests/e2e/billing/ledger/charge/test_async_ledger_charge.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/billing/ledger/conftest.py (1)
ledger_id(5-6)tests/e2e/billing/ledger/charge/conftest.py (2)
ledger_charge_id(5-6)invalid_ledger_charge_id(10-11)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/billing/ledger/attachment/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.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)
mpt_ops(31-32)pdf_fd(73-75)tests/e2e/billing/ledger/attachment/conftest.py (3)
ledger_attachment_factory(15-24)ledger_attachment_id(10-11)invalid_ledger_attachment_id(5-6)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
⏰ 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/e2e/billing/ledger/charge/test_async_ledger_charge.py (1)
1-43: LGTM! Well-structured async test module.The test module follows established patterns in the codebase:
- Proper use of
async/awaitthroughout- Correct async iteration with
async forintest_filter_ledger_charges- Appropriate use of
pytest.mark.flakyfor e2e tests- The hardcoded
"INV12345"is consistent with seeded test data conventions (based on learnings)tests/e2e/conftest.py (1)
161-163: LGTM! Appropriate fixture relocation.Moving
commerce_product_idto the root e2e conftest enables broader reuse across billing and commerce test modules. The implementation follows the established pattern used by other config-based fixtures in this file.tests/e2e/billing/ledger/test_sync_ledger.py (1)
1-26: LGTM! Good test coverage for ledger operations.Tests appropriately cover the main ledger operations: get by ID, list with pagination, 404 handling, and filtering. The tests follow the read-only pattern, reusing existing seeded resources to improve execution speed. Based on learnings, this is the preferred approach for non-destructive operations.
tests/e2e/billing/ledger/attachment/conftest.py (1)
1-24: LGTM! Clean fixture definitions.The fixtures follow established patterns in the e2e test suite:
invalid_ledger_attachment_idprovides a known-invalid ID for 404 testsledger_attachment_idretrieves the seeded attachment ID from configledger_attachment_factoryprovides a simple factory for creating attachment payloadstests/e2e/billing/ledger/attachment/test_sync_ledger_attachment.py (4)
9-26: LGTM! Proper fixture with cleanup handling.The
created_ledger_attachmentfixture correctly implements:
- Resource creation with the factory pattern
- Yielding for test use
- Safe teardown with exception handling for already-deleted resources
Based on learnings, this isolated fixture approach is appropriate for destructive tests (create/update/delete) that require per-test creation and cleanup.
33-62: LGTM! Read-only tests properly use shared resources.The get, list, filter, and download tests appropriately reuse seeded test data via
ledger_attachment_idandledger_attachmentsfixtures, following the pattern of optimizing test execution speed for non-destructive operations.
89-93: LGTM! Download test validates file content and metadata.The test correctly asserts both
file_contentsandfilenameare not None, verifying the download operation returns valid data with proper Content-Disposition headers.
71-87: Remove test interdependency concern—each test receives its own fixture instance.The
created_ledger_attachmentfixture is function-scoped (default), sotest_update_billing_ledger_attachmentandtest_delete_billing_ledger_attachmenteach receive a separate instance. The fixture also includes teardown cleanup that deletes the attachment after each test completes. Test order has no impact because a fresh instance is created for each test.Likely an incorrect or invalid review comment.
|
Added e2e tests for billing ledger, charges, and ledger attachments
Closes MPT-14915