[MPT-14920] Added e2e tests for invoice #192
Conversation
📝 WalkthroughWalkthroughIntroduces new end-to-end test fixtures and test suites for billing invoice operations. Adds a conftest fixture for an invalid invoice identifier and creates separate synchronous and asynchronous test modules covering invoice retrieval, error handling, listing, and filtering scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
robcsegal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/e2e/billing/invoice/test_async_invoice.py (2)
37-42: Consider adding skip logic for consistency with other tests.Unlike
test_get_invoice_by_idandtest_filter_invoices, this test will fail if no invoices exist rather than skip. Given that test data cannot be seeded (per PR description), consider handling the empty case consistently:async def test_list_invoices(async_mpt_ops): limit = 10 result = await async_mpt_ops.billing.invoices.fetch_page(limit=limit) - assert len(result) > 0 + if len(result) == 0: + pytest.skip("No invoices available for listing test.") + assert len(result) <= limitAlternatively, if asserting non-empty data is intentional to catch environment issues, the current approach is fine—just noting the inconsistency.
45-59: Minor: Variable shadowing in list comprehension.The loop variable
invoiceon line 57 shadows the fixture parameterinvoice. While this doesn't cause a bug (the fixture isn't used after this point), it reduces readability.- result = [invoice async for invoice in filtered_invoices.iterate()] + result = [inv async for inv in filtered_invoices.iterate()]tests/e2e/billing/invoice/test_sync_invoice.py (2)
37-42: Consider adding skip logic for consistency with other tests.Same observation as the async counterpart—this test will fail rather than skip if no invoices exist:
def test_list_invoices(mpt_ops): limit = 10 result = mpt_ops.billing.invoices.fetch_page(limit=limit) - assert len(result) > 0 + if len(result) == 0: + pytest.skip("No invoices available for listing test.") + assert len(result) <= limit
45-59: Minor: Variable shadowing in list comprehension.Same as the async version—the loop variable
invoiceshadows the fixture parameter:- result = list(filtered_invoices.iterate()) + result = list(inv for inv in filtered_invoices.iterate())Or simply rename the loop variable in the iteration context if
iterate()is used directly in a list comprehension elsewhere.
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/billing/invoice/conftest.pytests/e2e/billing/invoice/test_async_invoice.pytests/e2e/billing/invoice/test_sync_invoice.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/invoice/test_async_invoice.pytests/e2e/billing/invoice/test_sync_invoice.pytests/e2e/billing/invoice/conftest.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/invoice/test_async_invoice.pytests/e2e/billing/invoice/test_sync_invoice.pytests/e2e/billing/invoice/conftest.py
🧠 Learnings (5)
📓 Common learnings
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:26.691Z
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.
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:13.551Z
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-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/invoice/test_async_invoice.pytests/e2e/billing/invoice/test_sync_invoice.pytests/e2e/billing/invoice/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/invoice/test_async_invoice.pytests/e2e/billing/invoice/test_sync_invoice.pytests/e2e/billing/invoice/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/invoice/test_async_invoice.pytests/e2e/billing/invoice/test_sync_invoice.pytests/e2e/billing/invoice/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/billing/invoice/conftest.py
🧬 Code graph analysis (2)
tests/e2e/billing/invoice/test_async_invoice.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)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/billing/invoice/conftest.py (1)
invalid_invoice_id(5-6)
tests/e2e/billing/invoice/test_sync_invoice.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)
mpt_ops(31-32)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/billing/invoice/conftest.py (1)
invalid_invoice_id(5-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
tests/e2e/billing/invoice/conftest.py (1)
1-6: LGTM!Simple fixture for invalid invoice ID testing. The placeholder format follows the expected invoice ID pattern and will reliably trigger 404 responses for negative test cases.
tests/e2e/billing/invoice/test_async_invoice.py (4)
1-6: LGTM!Imports and flaky marker are appropriate for e2e tests that depend on external data availability.
9-19: LGTM!Good use of the sync fixture pattern for
invoicethat depends on the asyncinvoicesfixture. Based on learnings, pytest-asyncio will resolve the async fixture automatically, and this avoids linter complaints about unnecessary async functions.
22-29: LGTM!Appropriate skip logic when no test data is available.
32-34: LGTM!Clean 404 error handling test using the shared
invalid_invoice_idfixture.tests/e2e/billing/invoice/test_sync_invoice.py (4)
1-6: LGTM!Imports and flaky marker are consistent with the async test module.
9-19: LGTM!Fixtures follow the established pattern for reusing existing API resources for read-only operations, which improves test execution speed. Based on learnings, this is the preferred approach for non-destructive e2e tests.
22-29: LGTM!Correctly handles missing test data with skip logic.
32-34: LGTM!Consistent 404 handling test pattern.
|
Added e2e tests for invoice
Data can't be seeded because it comes from Nav.
https://softwareone.atlassian.net/browse/MPT-14920
Closes MPT-14920
Changes
invalid_invoice_idinconftest.pyto provide a consistent invalid invoice identifier for testing error cases