[MPT-14923] Added e2e tests for commerce credit memos #191
Conversation
📝 WalkthroughWalkthroughAdds end-to-end test coverage for credit memo operations: a new invalid ID fixture and separate async and sync test modules that verify fetching by ID (including 404), listing, and filtering credit memos. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 0
🧹 Nitpick comments (4)
tests/e2e/billing/credit_memo/test_async_credit_memo.py (2)
22-27: Consider skipping the test when no credit memos exist.If
credit_memoisNone, the test proceeds with an empty string ID, which may cause an unexpected error rather than a meaningful test failure. Consider usingpytest.skipto make the test behavior clearer when no data exists.💡 Suggested improvement
async def test_get_credit_memo_by_id(async_mpt_ops, credit_memo): - credit_memo_id = credit_memo.id if credit_memo else "" + if credit_memo is None: + pytest.skip("No credit memos available in the system") + credit_memo_id = credit_memo.id result = await async_mpt_ops.billing.credit_memos.get(credit_memo_id) assert result is not None
43-55: Same skip pattern could be applied here for consistency.The filter test has the same None-handling pattern. Consider applying the
pytest.skipapproach here as well for consistency.💡 Suggested improvement
async def test_filter_credit_memos(async_mpt_ops, credit_memo): - credit_memo_id = credit_memo.id if credit_memo else "" - credit_memo_status = credit_memo.status if credit_memo else "" + if credit_memo is None: + pytest.skip("No credit memos available in the system") + credit_memo_id = credit_memo.id + credit_memo_status = credit_memo.status select_fields = ["-vendor"]tests/e2e/billing/credit_memo/test_sync_credit_memo.py (2)
22-27: Same skip pattern suggestion applies here.For consistency with async tests, consider using
pytest.skipwhencredit_memoisNone.💡 Suggested improvement
def test_get_credit_memo_by_id(mpt_ops, credit_memo): - credit_memo_id = credit_memo.id if credit_memo else "" + if credit_memo is None: + pytest.skip("No credit memos available in the system") + credit_memo_id = credit_memo.id result = mpt_ops.billing.credit_memos.get(credit_memo_id) assert result is not None
43-55: Same skip pattern could be applied here for consistency.💡 Suggested improvement
def test_filter_credit_memos(mpt_ops, credit_memo): - credit_memo_id = credit_memo.id if credit_memo else "" - credit_memo_status = credit_memo.status if credit_memo else "" + if credit_memo is None: + pytest.skip("No credit memos available in the system") + credit_memo_id = credit_memo.id + credit_memo_status = credit_memo.status select_fields = ["-vendor"]
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/billing/credit_memo/conftest.pytests/e2e/billing/credit_memo/test_async_credit_memo.pytests/e2e/billing/credit_memo/test_sync_credit_memo.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/credit_memo/test_async_credit_memo.pytests/e2e/billing/credit_memo/conftest.pytests/e2e/billing/credit_memo/test_sync_credit_memo.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/credit_memo/test_async_credit_memo.pytests/e2e/billing/credit_memo/conftest.pytests/e2e/billing/credit_memo/test_sync_credit_memo.py
🧠 Learnings (4)
📓 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: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.
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.
📚 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/credit_memo/test_async_credit_memo.pytests/e2e/billing/credit_memo/conftest.pytests/e2e/billing/credit_memo/test_sync_credit_memo.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/credit_memo/test_async_credit_memo.pytests/e2e/billing/credit_memo/conftest.pytests/e2e/billing/credit_memo/test_sync_credit_memo.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/credit_memo/test_async_credit_memo.pytests/e2e/billing/credit_memo/conftest.pytests/e2e/billing/credit_memo/test_sync_credit_memo.py
🧬 Code graph analysis (1)
tests/e2e/billing/credit_memo/test_sync_credit_memo.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/credit_memo/conftest.py (1)
invalid_credit_memo_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/credit_memo/conftest.py (1)
1-6: LGTM!Clean and straightforward fixture for negative test cases. The ID format follows the expected credit memo pattern and provides a consistent invalid ID for 404 scenarios across both async and sync tests.
tests/e2e/billing/credit_memo/test_async_credit_memo.py (4)
1-6: LGTM!Appropriate imports and the
flakymarker is suitable for e2e tests that depend on external systems and pre-existing data.
9-19: LGTM!The
credit_memofixture correctly usesdefinstead ofasync defsince it contains noawaitexpressions, and pytest-asyncio handles the async fixture resolution automatically. Based on learnings, this follows the team's established pattern.
30-32: LGTM!Clean negative test case that properly validates the 404 response for a non-existent credit memo.
35-40: LGTM!Straightforward list test. The
flakymarker appropriately accounts for the dependency on pre-existing data.tests/e2e/billing/credit_memo/test_sync_credit_memo.py (4)
1-6: LGTM!Appropriate imports and
flakymarker consistent with the async test module.
9-19: LGTM!Fixtures appropriately reuse existing API resources for read-only operations, following the team's pattern for e2e test efficiency. Based on learnings, this approach is intentional.
30-32: LGTM!Properly validates the 404 response for a non-existent credit memo ID.
35-40: LGTM!The list test is appropriately marked flaky and follows the team's pattern for e2e tests.
robcsegal
force-pushed
the
MPT-14923-add-e-2-e-tests-for-billing-credit-memos
branch
from
be0c9fc to
644a970
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/e2e/billing/credit_memo/test_sync_credit_memo.py:
- Around line 37-42: test_list_credit_memos currently asserts there are results
but doesn't handle an empty system; update the test to mirror other tests by
importing pytest if needed and adding a check after calling
mpt_ops.billing.credit_memos.fetch_page(limit=limit): if len(result) == 0 then
call pytest.skip("no credit memos available") so the test is skipped in
environments with no data (alternatively change the assertion to assert
len(result) >= 0 if empty results are acceptable).
🧹 Nitpick comments (1)
tests/e2e/billing/credit_memo/test_sync_credit_memo.py (1)
22-29: Consider strengthening the assertion.The test verifies the result is not
None, but could also verify that the returned credit memo's ID matches the requested ID for more thorough validation.💡 Suggested improvement
result = mpt_ops.billing.credit_memos.get(credit_memo_id) - assert result is not None + assert result is not None + assert result.id == credit_memo_id
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/billing/credit_memo/conftest.pytests/e2e/billing/credit_memo/test_async_credit_memo.pytests/e2e/billing/credit_memo/test_sync_credit_memo.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/billing/credit_memo/test_async_credit_memo.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/credit_memo/test_sync_credit_memo.pytests/e2e/billing/credit_memo/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/credit_memo/test_sync_credit_memo.pytests/e2e/billing/credit_memo/conftest.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/credit_memo/test_sync_credit_memo.pytests/e2e/billing/credit_memo/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/credit_memo/test_sync_credit_memo.pytests/e2e/billing/credit_memo/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/credit_memo/test_sync_credit_memo.pytests/e2e/billing/credit_memo/conftest.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
tests/e2e/billing/credit_memo/conftest.py (1)
1-6: LGTM!Clean fixture providing a well-formed invalid ID for 404 testing. The
CRD-prefix follows the expected credit memo ID format.tests/e2e/billing/credit_memo/test_sync_credit_memo.py (3)
1-6: LGTM!Imports are clean and the module-level
flakymarker correctly aligns with project conventions for e2e tests as per pyproject.toml configuration.
9-19: LGTM - fixtures follow the read-only resource reuse pattern.Based on learnings, reusing existing API resources for read-only operations is the correct approach here. The
Nonehandling incredit_memoproperly supports skip logic in dependent tests.Consider moving these fixtures to
conftest.pyif the async test module needs similar fixtures, to avoid duplication.
45-59: LGTM!Well-structured filter test with appropriate skip logic. The chained filter calls with
RQLQueryand field exclusion viaselectdemonstrate comprehensive API coverage.
|
robcsegal
deleted the
MPT-14923-add-e-2-e-tests-for-billing-credit-memos
branch
Added e2e tests for commerce credit memos
Data can't be seeded because it comes from Nav, but operations have data already there. Not ideal, but it's something.
https://softwareone.atlassian.net/browse/MPT-14923
Closes MPT-14923