[MPT-14918] Added e2e tests for billing statements and statement charges #189
Conversation
📝 WalkthroughWalkthroughThis PR increases GitHub Actions workflow timeouts from 45 to 60 minutes across four workflow files and adds comprehensive end-to-end test coverage for billing statements and statement charges with new configuration entries, fixtures, and async/sync test implementations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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: 1
🤖 Fix all issues with AI agents
In @tests/e2e/billing/statement/charge/test_sync_statement_charge.py:
- Around line 14-17: The test test_get_statement_charge_by_id currently only
asserts that result is not None; update it to verify the returned charge has the
expected identifier by asserting the ID on the fetched object equals
statement_charge_id (e.g., assert result.id == statement_charge_id or assert
result["id"] == statement_charge_id depending on how statement_charges.get
returns the object) to ensure the correct charge was retrieved.
🧹 Nitpick comments (6)
tests/e2e/billing/statement/conftest.py (1)
9-11: LGTM! Invalid fixture is clear and intentional.The hardcoded invalid statement ID is appropriate for negative testing. Based on learnings, hardcoded values in e2e tests are acceptable for seeded test data and testing error scenarios.
Optional: Consider adding a docstring to document that this ID format is intentionally invalid (all zeros).
📝 Optional documentation enhancement
@pytest.fixture def invalid_statement_id(): + """Returns an intentionally invalid statement ID for negative testing.""" return "SOM-0000-0000-0000-0000"tests/e2e/billing/statement/test_async_statement.py (1)
28-38: Avoid fully materializing filtered iterators when you expect a single result.This currently reads all results into memory; you can short-circuit and still assert uniqueness.
Proposed refactor
async def test_filter_statements(async_mpt_ops, statement_id): select_fields = ["-client"] filtered_statements = ( async_mpt_ops.billing.statements.filter(RQLQuery(id=statement_id)) .filter(RQLQuery(type="Debit")) .select(*select_fields) ) - - result = [statement async for statement in filtered_statements.iterate()] - - assert len(result) == 1 + items = [] + async for stmt in filtered_statements.iterate(): + items.append(stmt) + if len(items) > 1: + break + assert len(items) == 1tests/e2e/billing/statement/charge/test_async_statement_charge.py (1)
33-43: Make the seeded invoice external ID explicit + fix iterator variable naming.This improves readability and reduces brittleness when seed data changes. (Based on learnings, prefer named constants + document seed dependency.)
Proposed refactor
async def test_filter_statement_charges(statement_charges, statement_charge_id): + # Seeded test data dependency: update if seeds change. + INVOICE_EXTERNAL_ID = "INV12345" select_fields = ["-price"] filtered_charges = ( statement_charges.filter(RQLQuery(id=statement_charge_id)) - .filter(RQLQuery(externalIds__invoice="INV12345")) + .filter(RQLQuery(externalIds__invoice=INVOICE_EXTERNAL_ID)) .select(*select_fields) ) - result = [statement async for statement in filtered_charges.iterate()] + result = [charge async for charge in filtered_charges.iterate()] assert len(result) == 1tests/e2e/billing/statement/charge/conftest.py (1)
4-7: Consider a clearer error thanKeyErrorif the config key is missing.Optional, but tends to speed up CI debugging when
TEST_CONFIG_FILEpoints to a different config.Proposed tweak
@pytest.fixture def statement_charge_id(e2e_config): - return e2e_config["billing.statement.charge.id"] + key = "billing.statement.charge.id" + assert key in e2e_config, f"Missing required e2e config key: {key}" + return e2e_config[key]tests/e2e/billing/statement/test_sync_statement.py (1)
28-38: Avoid fully materializing iterators when you only assert a single result.Minor perf/readability win and matches the intent of the test.
Proposed refactor
def test_filter_statements(mpt_ops, statement_id): select_fields = ["-client"] filtered_statements = ( mpt_ops.billing.statements.filter(RQLQuery(id=statement_id)) .filter(RQLQuery(type="Debit")) .select(*select_fields) ) - - result = list(filtered_statements.iterate()) - - assert len(result) == 1 + items = [] + for stmt in filtered_statements.iterate(): + items.append(stmt) + if len(items) > 1: + break + assert len(items) == 1tests/e2e/billing/statement/charge/test_sync_statement_charge.py (1)
33-43: Extract hardcoded external ID to a named constant.The hardcoded external invoice ID
"INV12345"should be extracted to a named constant (e.g.,INVOICE_EXTERNAL_ID) and documented as dependent on seed data. This improves maintainability and makes the test data dependency explicit.Based on learnings, hardcoded external IDs in e2e tests are intentional because they come from seeded test data, but using named constants helps avoid brittle tests.
♻️ Suggested refactor
At the top of the file, after imports:
+# External IDs from seeded test data +INVOICE_EXTERNAL_ID = "INV12345" + pytestmark = [pytest.mark.flaky]Then update the test:
def test_filter_statement_charges(statement_charges, statement_charge_id): select_fields = ["-price"] filtered_charges = ( statement_charges.filter(RQLQuery(id=statement_charge_id)) - .filter(RQLQuery(externalIds__invoice="INV12345")) + .filter(RQLQuery(externalIds__invoice=INVOICE_EXTERNAL_ID)) .select(*select_fields) ) result = list(filtered_charges.iterate()) assert len(result) == 1
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/cron-main-e2e.yml.github/workflows/pull-request.yml.github/workflows/push-release-branch.yml.github/workflows/release.ymle2e_config.test.jsontests/e2e/billing/statement/charge/conftest.pytests/e2e/billing/statement/charge/test_async_statement_charge.pytests/e2e/billing/statement/charge/test_sync_statement_charge.pytests/e2e/billing/statement/conftest.pytests/e2e/billing/statement/test_async_statement.pytests/e2e/billing/statement/test_sync_statement.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
tests/e2e/billing/statement/conftest.pytests/e2e/billing/statement/charge/conftest.pytests/e2e/billing/statement/test_sync_statement.pye2e_config.test.jsontests/e2e/billing/statement/charge/test_async_statement_charge.pytests/e2e/billing/statement/charge/test_sync_statement_charge.pytests/e2e/billing/statement/test_async_statement.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections.
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.
Files:
tests/e2e/billing/statement/conftest.pytests/e2e/billing/statement/charge/conftest.pytests/e2e/billing/statement/test_sync_statement.pytests/e2e/billing/statement/charge/test_async_statement_charge.pytests/e2e/billing/statement/charge/test_sync_statement_charge.pytests/e2e/billing/statement/test_async_statement.py
🧠 Learnings (3)
📚 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/statement/conftest.pytests/e2e/billing/statement/charge/conftest.pytests/e2e/billing/statement/test_sync_statement.pytests/e2e/billing/statement/charge/test_async_statement_charge.pytests/e2e/billing/statement/charge/test_sync_statement_charge.pytests/e2e/billing/statement/test_async_statement.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/statement/conftest.pytests/e2e/billing/statement/charge/conftest.pytests/e2e/billing/statement/test_sync_statement.pytests/e2e/billing/statement/charge/test_async_statement_charge.pytests/e2e/billing/statement/charge/test_sync_statement_charge.pytests/e2e/billing/statement/test_async_statement.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/statement/conftest.pytests/e2e/billing/statement/charge/conftest.pytests/e2e/billing/statement/test_sync_statement.pytests/e2e/billing/statement/charge/test_async_statement_charge.pytests/e2e/billing/statement/charge/test_sync_statement_charge.pytests/e2e/billing/statement/test_async_statement.py
🧬 Code graph analysis (3)
tests/e2e/billing/statement/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/billing/statement/charge/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/billing/statement/charge/test_sync_statement_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/statement/charge/conftest.py (2)
statement_charge_id(5-6)invalid_statement_charge_id(10-11)mpt_api_client/models/model.py (1)
id(119-121)
⏰ 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)
.github/workflows/push-release-branch.yml (1)
16-16: LGTM! Timeout increase accommodates expanded test suite.The timeout increase from 45 to 60 minutes is appropriate given the addition of comprehensive e2e tests for billing statements and charges in this PR.
.github/workflows/cron-main-e2e.yml (1)
9-9: LGTM! Consistent timeout adjustment.The timeout increase aligns with changes across all workflow files and accommodates the expanded e2e test coverage.
.github/workflows/pull-request.yml (1)
14-14: LGTM! Appropriate timeout for PR validation.The extended timeout ensures the PR workflow has adequate time to complete with the newly added billing statement e2e tests.
.github/workflows/release.yml (1)
15-15: LGTM! Consistent timeout across all workflows.The timeout increase is appropriate and maintains consistency across all workflow files in this PR.
tests/e2e/billing/statement/conftest.py (2)
1-2: LGTM! Clean imports.The minimal import is appropriate for the fixture definitions.
4-6: LGTM! Fixture follows established pattern.The
statement_idfixture correctly reads from the e2e configuration, consistent with other e2e test fixtures in the codebase. Based on learnings, reusing existing API resources for read-only operations speeds up test execution.tests/e2e/billing/statement/test_sync_statement.py (1)
9-38: Nice: read-only e2e coverage + consistent 404 behavior checks.These tests appear non-destructive and should be fast/stable assuming seeded IDs stay consistent. (Based on learnings, reuse existing resources.)
tests/e2e/billing/statement/test_async_statement.py (1)
6-6: No action required—pytest-asyncio is correctly configured.The repository has
asyncio_mode = "auto"configured inpyproject.toml:70, which automatically handles async test functions without requiring explicit@pytest.mark.asynciomarkers. The tests in this module are correctly written.e2e_config.test.json (1)
24-25: No action needed — the hardcoded statement charge ID is intentional and validated by test structure.The test design in
test_sync_statement_charge.pyverifies the relationship: thestatement_chargesfixture scopes charges to a specific statement ID, andtest_get_statement_charge_by_idretrieves the charge within that scope. If the charge ID didn't belong to the statement, the test would fail with a 404. The hardcoded ID values in the e2e config come from seeded test data and are guaranteed to have correct relationships.Likely an incorrect or invalid review comment.
tests/e2e/billing/statement/charge/test_sync_statement_charge.py (4)
1-6: LGTM! Appropriate imports and test markers.The imports are well-chosen for e2e testing, and marking the module as flaky is appropriate for end-to-end tests that may experience transient failures.
9-11: LGTM! Fixture appropriately reuses existing resources.This fixture follows the e2e testing pattern of reusing existing API resources for read-only operations, which speeds up test execution.
20-22: LGTM! Proper error handling validation.The test correctly validates that attempting to retrieve a non-existent statement charge raises the expected 404 error.
25-30: LGTM! Basic pagination test.The test validates that the fetch_page operation returns results. The assumption that at least one charge exists is acceptable for e2e tests with seeded test data.
|
Added e2e tests for billing statements and statement charges
https://softwareone.atlassian.net/browse/MPT-14918
https://softwareone.atlassian.net/browse/MPT-14919
Closes MPT-14918
e2e_config.test.json